memory safe subscription callback #108
Conversation
Inherit from std::enable_shared_from_this and wrap subscription callback in lambda capturing a weak_ptr using weak_from_this() to check that Bond object is still in scope before processing the bondStatusCB member function.
Create and use helper function to check that class is still valid before calling member function.
|
Note that beyond the linting problems that the job points out, there's an actual set of test failures in |
This is due to the use of createSafeSubscriptionMemFuncCallback when creating the subscription registering the bondStatusCB member function
|
@SteveMacenski fixed the lint and test. This change requires the Bond to be created as a shared_ptr. |
| msg->heartbeat_timeout = static_cast<float>(heartbeat_timeout_.seconds()); | ||
| msg->heartbeat_period = static_cast<float>(heartbeat_period_.seconds()); | ||
| if (pub_->get_subscription_count() < 1) {return;} | ||
| pub_->publish(std::move(msg)); |
There was a problem hiding this comment.
@SteveMacenski This is another use_intraprocess_comms fix. i.e the Bond Status publisher for use_intraprocess_comms should use a unique msg and check for subscribers.
This allows nav2_system_tests to pass.
NOTE: without this change the following test was failure was very reproducible for me.
colcon test --packages-select nav2_system_tests --event-handlers=console_direct+ --ctest-args -R test_speed_filter_global
There was a problem hiding this comment.
Does this make it pass because of the if (pub_->get_subscription_count() < 1) {return;} return condition or the unique_ptr publisher? I'm curious if this is pattern I should be on the look-out for on MsgT publishers for depedencies, or if this is a special case where we can get around it by skipping publication.
Though this fixes the issue in Bond, if its the latter, I think that should be reported in the rclcpp thread that publishing IPC message instances causes part of the issue. That means something in the publish() for MsgT signatures is broken whereas MsgT::UniquePtr are not (which hopefully would be an easy apples-to-apples fix).
|
Just a note that these changes are ABI-breaking (adding members and changing inheritance), so definitely need to land in rolling. |
|
I know that the aim is really to fix rclcpp, not this - so I think this PR is mostly used for an experimental reproduction ground and testing possible application-space solutions for the rclcpp IPC issue. @ewak : Is this intended to be a candidate for merging for either a short term solution to get Nav2 turning over with IPC and/or should be reviewed by maintainers for inclusion? Maintainers: This requires creating the object as a shared pointer, similar to rclcpp nodes themselves. Would this be acceptable to maintainers to put that restriction for getting IPC running on nodes that use bond? I'd like to bridge the gap a bit to figure out next steps here, if there are some outside of rclcpp |
Manage the lifetime of the bondStatusCB member function by wrapping it in a lambda that captures a weak_ptr (via public inheritance of std::enable_shared_from_this)
NOTE:
This is a draft to show how it could be done.
It solves a use after free that is being experienced using intraprocess communication in nav2 ros-navigation/navigation2#4691 .
Also being discussed as a bug in rclcpp - ros2/rclcpp#2678 (comment)
Something like createSafeSubscriptionMemFuncCallback could make its way into the rclcpp namespace.