ros2 / examples

Example packages for ROS 2
Apache License 2.0
681 stars 308 forks source link

Use a single executor instance for spinning in client_async_callback. #382

Closed clalancette closed 2 months ago

clalancette commented 2 months ago

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.

clalancette commented 2 months ago

CI:

clalancette commented 2 months ago

(maybe we would keep this follow-up on rmw_cyclonedds repo. it is easy to reproduce to revert this fix.)

Yeah, good point. I'll open an issue on https://github.com/ros2/rmw_cyclonedds to follow-up with that.

clalancette commented 2 months ago

See https://github.com/ros2/rmw_cyclonedds/issues/494

wjwwood commented 2 months ago

@ros-pull-request-builder retest this please

wjwwood commented 2 months ago

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/

clalancette commented 2 months ago

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!

clalancette commented 2 months ago

@Mergifyio backport jazzy

mergify[bot] commented 2 months ago

backport jazzy

✅ Backports have been created

* [#385 Use a single executor instance for spinning in client_async_callback. (backport #382)](https://github.com/ros2/examples/pull/385) has been created for branch `jazzy` but encountered conflicts