graph_listener cleaning memory managed by parent context #1340
Description
I found this issue when adding coverage tests for rclcpp, PR: #1330.
This is the causing problems, just in MacOS:
auto context_to_destroy = std::make_shared<rclcpp::contexts::DefaultContext>();
context_to_destroy->init(0, nullptr);
auto graph_listener_error =
std::make_shared<rclcpp::graph_listener::GraphListener>(context_to_destroy);
context_to_destroy.reset();
RCLCPP_EXPECT_THROW_EQ(
graph_listener_error->start_if_not_started(),
std::runtime_error("parent context was destroyed"));Expected behavior
graph_listener is destroyed cleanly
Actual behavior
graph_listener causes segfault when calling its shutdown method (destruction).
The portion of code causing a problem is:
if (shutdown_guard_condition_) {
auto parent_context_ptr = parent_context_.lock();
if (parent_context_ptr) {
if (should_throw) {
parent_context_ptr->release_interrupt_guard_condition(&wait_set_);
} else {
parent_context_ptr->release_interrupt_guard_condition(&wait_set_, std::nothrow);
}
} else {
ret = rcl_guard_condition_fini(shutdown_guard_condition_);
if (RCL_RET_OK != ret) {
if (should_throw) {
throw_from_rcl_error(ret, "failed to finalize shutdown guard condition");
} else {
RCLCPP_ERROR(
rclcpp::get_logger("rclcpp"),
"failed to finalize shutdown guard condition");
}
}
}
shutdown_guard_condition_ = nullptr;The shutdown_guard_condition_ memory is allocated at the context level here so I think it shouldn't be fini at the graph_listener, it should be fini at the context level, and graph_listener would just check the weak pointer referencing the parent context ptr. This would change this portion of code to be:
auto parent_context_ptr = parent_context_.lock();
if (parent_context_ptr) {
if (should_throw) {
parent_context_ptr->release_interrupt_guard_condition(&wait_set_);
} else {
parent_context_ptr->release_interrupt_guard_condition(&wait_set_, std::nothrow);
}
shutdown_guard_condition_ = nullptr;
}Not sure if there would be needed an additional check for shutdown_guard_condition_ though. Let me know what you think of this issue and what would be an appropriate fix.
Reference build: https://ci.ros2.org/job/ci_osx/10203/testReport/junit/projectroot/test/test_graph_listener/