DRAFT: Fix: Double spin required since 28.2.0

[jmachowinski]

This is a first attempt to fix #2589.

Rebuild the collection explicitly before the wait, if the notify_waitable_ has been triggered.

[jmachowinski]

@alsora can you run the CI on this ?

[alsora]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ros-discourse]

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

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-16th-august-2024/39130/1

[MichaelOrlov]

@jmachowinski A friendly ping to fix CI failures to move forward with this PR.

[jmachowinski]

@MichaelOrlov we spoke about this bug in the last client workgroup meeting. We don't think this is the correct fix, but a step into the right direction.Unfortunately I currently don't have much time I can divert to rclcpp.

[alsora]

PR https://github.com/ros2/rclcpp/pull/2591 merged. That PR introduces unit-tests and fixes this problem for the events executor.

I looked a bit more into the waitset executor issue, and IMO even if this PR is likely not the best way to address the problem, it definitely improves the situation in a relatively simple way, fixing the bug reported by the nav stack and passing the unit tests that I added in the other PR.

My recommendation is to:

• move the new logic to a dedicated function
• run this new function only for spin some, spin once and spin all, rather than in each wait for work (e.g. by adding a bool flag to wait for work). spin does not need it, and it would only cause unnecessary overhead.
• enable the single threaded executor in the new unit tests.

If people agree, I can take care of pushing these changes (or @jmachowinski can do it if he has time).

[jmachowinski]

closing in favor of https://github.com/ros2/rclcpp/pull/2724