ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
562 stars 425 forks source link

Honor the user holding onto shared_ptrs during subscription callbacks #2401

Open clalancette opened 10 months ago

clalancette commented 10 months ago

While debugging https://github.com/ros2/rmw_cyclonedds/issues/469 , we found that loaned messages do not properly honor the user holding onto shared_ptrs. In particular, if you look at the code in https://github.com/ros2/rclcpp/blob/126d517193b6df8177680898e23906a85792eaf6/rclcpp/src/rclcpp/executor.cpp#L626-L662 , you can see that the executor always immediately returns the loan to the rmw layer, regardless of whether the user took a shared_ptr reference.

We have worked around this for now by disabling loans by default, but that is not an ideal solution. Instead, what we should do is make a custom destructor for the shared_ptr that we create, which is responsible for returning the loan to the rmw layer. With that in place, we could turn loans back on, and still be safe when users take shared_ptr references.

sgf201 commented 10 months ago

rmw_cyclonedds_cpp needs a deep refact to solve the allocator problem if making a custom destructor for the shared_ptr https://github.com/eclipse-iceoryx/iceoryx/issues/2159

ros-discourse commented 6 months ago

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/client-library-wg-meeting/37915/1

wjwwood commented 5 months ago

@sgf201 how is an allocator involved at this level? What @clalancette is mentioning is the logic in our loaned message wrapper in C++, which just wraps the rmw layer. I don't think an allocator needs to be passed from user space to the rmw impl or vice versa. The allocation of the loaned message's memory is completely up to the rmw implementation.

sgf201 commented 5 months ago

@wjwwood I agree with your point, this issue should indeed be handled at the RMW layer. I'm not very familiar with the other RMW implementations, but in rmw_cyclonedds_cpp, they have implemented the loan API using iceoryx. Additionally, for serialized messages within the same host, they also use iceoryx to pass the serialized messages. This means the subscribers may receive two types of messages - the loan messages that do not require deserialization, and the messages passed through iceoryx that need to be deserialized,and then stored in heap. The handling of message space release differs between these two types of messages when returning the shared pointers. This means that when fully implementing the shared pointer lifecycle, the issue of proper message space release needs to be carefully handle. I thought that allowing iceoryx subscribers to loan buffer may have been a relatively simple way to simplify the implementation of rmw_cyclonedds_cpp. However, this approach indeed should not involve iceoryx considering it. https://github.com/eclipse-iceoryx/iceoryx/issues/2159