Disable the loaned messages inside the executor.

[clalancette]

They are currently unsafe to use; see the comment in the commit for more information.

This was originally brought up by ros2/rmw_cyclonedds#469 , but we also had another report of it from Nav2 (parts reproduced here for context):

From @SteveMacenski

*    It looks like Fast-DDS returns the same shared pointer memory for multiple subscriber callbacks in a row
*    We take the shared_ptr message from the callback and store it for use. So later on, the community member is identifying that Fast-DDS sets the same underlying memory in msg to the new message, even though we have stored the shared pointer in our code and just reusing it, wiping away our program's state

From @wjwwood:

This is probably a bug in rclcpp: https://github.com/ros2/rclcpp/blob/rolling/rclcpp/src/rclcpp/executor.cpp#L626-L662

[fujitatomoya]

@clalancette i am not sure if you had the issue, but i guess this one is known constraint originally.

https://github.com/ros2/rcl/blob/e37d7f8532c287df0fd8d3727677f5dde20dd5f3/rcl/include/rcl/subscription.h#L641-L644

which i do agree that is still unsafe, because user still would copy the message in the user callback. (i think that is your concern.)

(i think you put this as draft because of this.) i am not sure if we can forcibly disable this feature on subscription, since it will disable entire zero copy feature with ROS 2. if we go that way, i feel like we should never have enabled can_loan_message on the subscription... 😓

[wjwwood]

Lgtm, as we discussed it offline.

With one comment about how the todo could be worded more precisely.

[wjwwood]

The users can still take loaned messages from subscriptions manually but it will effectively disable zero copy for callbacks until we make more changes.

[wjwwood]

Also we should cross reference the existing issue and add the context we got from the Nav2 users here so that future us knows what is going on 😅

[clalancette]

which i do agree that is still unsafe, because user still would copy the message in the user callback. (i think that is your concern.)

Yes, that is the major concern. The original issue mentioned here is ros2/rmw_cyclonedds#469 , though we had another report on it today.

(i think you put this as draft because of this.) i am not sure if we can forcibly disable this feature on subscription, since it will disable entire zero copy feature with ROS 2. if we go that way, i feel like we should never have enabled can_loan_message on the subscription... 😓

The problem is that right now, shared_ptr subscriptions are super dangerous to use. Users who take a reference to the shared_ptr inside the callback (rightly) expect that the object will persist, but that is not the case with this automatic loaning. So I kind of think we have to make this change for now.

That said, we can talk about options for reenabling it in the future. That's why this draft is just disabling the feature, not removing it.

[clalancette]

Also we should cross reference the existing issue and add the context we got from the Nav2 users here so that future us knows what is going on 😅

Good point. I'll modify the initial comment to have that context.

[clalancette]

While playing around with this, I also noticed another nasty problem with loaned messages. In particular, if you set a custom memory strategy in your subscription with the current code, like:

  auto msg_pool_strategy = std::make_shared<rclcpp::strategies::message_pool_memory_strategy::MessagePoolMemoryStrategy<pendulum_msgs::msg::JointState, 10>>();
  auto sub = node->create_subscription<pendulum_msgs::msg::JointState>("myjointstate", qos, callback, rclcpp::SubscriptionOptions(), msg_pool_strategy);

That memory pool strategy will be utterly ignored by the loaned message interface here. I guess it kind of makes sense, since the "pool" is coming from the underlying RMW. But if that is the case, we should throw an exception or something when the user tries to set a custom one. Otherwise it is confusing why the strategy is not being used.

[fujitatomoya]

@clalancette @wjwwood thanks for the explanation. i am not happy with this 💦 but so true...

how about this approach? cz forcibly and statically hard-code disabling can be surprising (current user who understands this constrain) and leaves us no choice instead of rebuilding.

diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c
index 1ca6edd..9b24852 100644
--- a/rcl/src/rcl/subscription.c
+++ b/rcl/src/rcl/subscription.c
@@ -233,7 +233,7 @@ rcl_subscription_get_default_options()
   default_options.rmw_subscription_options = rmw_get_default_subscription_options();

   // Load disable flag to LoanedMessage via environmental variable.
-  bool disable_loaned_message = false;
+  bool disable_loaned_message = true;
   rcl_ret_t ret = rcl_get_disable_loaned_message(&disable_loaned_message);
   if (ret == RCL_RET_OK) {
     default_options.disable_loaned_message = disable_loaned_message;
@@ -241,7 +241,7 @@ rcl_subscription_get_default_options()
     RCUTILS_SAFE_FWRITE_TO_STDERR("Failed to get disable_loaned_message: ");
     RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str);
     rcl_reset_error();
-    default_options.disable_loaned_message = false;
+    default_options.disable_loaned_message = true;
   }

   return default_options;

that said, if user want to use loaned interfaces on the subscription, user must set export ROS_DISABLE_LOANED_MESSAGES=0, originally this environmental variable is meant to disable but this can be option?

[fujitatomoya]

@MiguelCompany @Barry-Xu-2018 @iuhilnehc-ynos you might want to keep eyes on this.

[clalancette]

So the latest commit undoes the work here. Instead, all this does now is to print a warning if loaned messages are enabled. This now needs to go in with ros2/rcl#1110 , which is where we actually do the disabling.

[clalancette]

Here is full CI on the pair, just to make sure things work as we expect:

• Linux
• Linux-aarch64
• Windows

[SteveMacenski]

While I think we should outright disable it if its not safe (for the time being until it is safe), I think this is also reasonable if the performance hit on disabling it always is super significant.

[clalancette]

While I think we should outright disable it if its not safe (for the time being until it is safe), I think this is also reasonable if the performance hit on disabling it always is super significant.

I ended up doing it this way because there are ways to use it safely. In particular if you use the default message strategy and only use ConstRef callback types, it is safe to use. That is a common use case, so users who really know what they are doing could opt-in to this to get slightly better performance.

But since there are a bunch of other scenarios that users could use (including using the shared_ptr callback type), it should be disabled by default (at least, until we fix it).

[ros-discourse]

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-10-12/34178/1

[clalancette]

CI for this is in ros2/rcl#1110 (comment)

[clalancette]

@Mergifyio backport humble iron

[mergify]

backport humble iron

✅ Backports have been created

• #2364 Disable the loaned messages inside the executor. (backport #2335) has been created for branch humble but encountered conflicts
• #2365 Disable the loaned messages inside the executor. (backport #2335) has been created for branch iron

[ros-discourse]

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

https://discourse.ros.org/t/preparing-for-humble-sync-and-patch-release-2023-11-17/34628/3

[ros-discourse]

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

https://discourse.ros.org/t/important-announcement-patch-release-3-and-new-packages-for-iron-irwini-2023-11-20/34742/1

[ros-discourse]

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

https://discourse.ros.org/t/preparing-for-iron-patch-and-sync-2023-11-17/34609/2