Use a single executor instance for spinning in client_async_callback. #382
Conversation
In the current code, it calls rclpy.spin_once(), which instantiates a new executor, adds the node to it, executors one work item, then removes the node and destroys the executor. Besides being inefficient, the problem with that sequence is that adding a node to the executor actually ends up being an "event", and so the work item that gets returned can be just the work of adding the node, over and over again. For reasons I admit I don't fully understand, this only happens with rmw_cyclonedds_cpp, not with rmw_fastrtps_cpp. Regardless, the much more performant thing to do is to create an executor at the beginning of the example and to just spin on that. This eliminates adding it to the node constantly, and makes this work with all RMWs. Signed-off-by: Chris Lalancette <[email protected]>
There was a problem hiding this comment.
the fix lgtm, but i cannot explain why this is happening with cyclonedds either.
it never takes the service response from the server, server has sent the service response to the client for sure. (maybe we would keep this follow-up on rmw_cyclonedds repo. it is easy to reproduce to revert this fix.)
|
CI:
|
Yeah, good point. I'll open an issue on https://github.com/ros2/rmw_cyclonedds to follow-up with that. |
There was a problem hiding this comment.
lgtm
I am digging into why this happens only with rmw_cyclonedds. As you pointed out in ros2/rmw_cyclonedds#494 @clalancette, it's due to a single kind of event being processed each time you spin which is never the event you want.
I think this change is just better though. The "global" version of spin_once() is kind of not that useful and in this case harmful, at least in my opinion. We could consider deprecating and then removing it. But the issue with cyclone should still be followed up on.
|
@ros-pull-request-builder retest this please |
|
I think the Rpr failure is an unrelated (probably pre-existing) failure: https://build.ros2.org/job/Rpr__examples__ubuntu_noble_amd64/19/testReport/launch_testing_examples.launch_testing_examples/record_rosbag_launch_test/launch_testing_examples_record_rosbag_launch_test/ |
Yeah, agreed. I'm going to go ahead and merge this in, thanks for the reviews! |
delete-merged-branch
bot
deleted the
clalancette/fix-client-async-callback
branch
|
@Mergifyio backport jazzy |
✅ Backports have been created
|
…#382) In the current code, it calls rclpy.spin_once(), which instantiates a new executor, adds the node to it, executors one work item, then removes the node and destroys the executor. Besides being inefficient, the problem with that sequence is that adding a node to the executor actually ends up being an "event", and so the work item that gets returned can be just the work of adding the node, over and over again. For reasons I admit I don't fully understand, this only happens with rmw_cyclonedds_cpp, not with rmw_fastrtps_cpp. Regardless, the much more performant thing to do is to create an executor at the beginning of the example and to just spin on that. This eliminates adding it to the node constantly, and makes this work with all RMWs. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit 7e47aee) # Conflicts: # rclpy/services/minimal_client/examples_rclpy_minimal_client/client_async_callback.py
In the current code, it calls rclpy.spin_once(), which instantiates a new executor, adds the node to it, executors one work item, then removes the node and destroys the executor.
Besides being inefficient, the problem with that sequence is that adding a node to the executor actually ends up being an "event", and so the work item that gets returned can be just the work of adding the node, over and over again. For reasons I admit I don't fully understand, this only happens with rmw_cyclonedds_cpp, not with rmw_fastrtps_cpp.
Regardless, the much more performant thing to do is to create an executor at the beginning of the example and to just spin on that. This eliminates adding it to the node constantly, and makes this work with all RMWs.