Ensure waitables handle guard condition retriggering

[wjwwood]

Re-opened version of https://github.com/ros2/rclcpp/pull/2420 (it seems the target branch being deleted prevents me from reopening it and/or updating the target branch...).

Copied from the original description:

An emerging responsibility of Waitables that wasn't explicitly declared before is that they should be ensuring guard conditions should stay ready between waits so long as the condition for them being triggered is still true.

Some waitables will not need this, e.g. waitables that are used to only wake up an executor once, but don't have events tied to them don't need to be retriggered. But other waitables which use guard conditions to notify the executor of work to be done need to keep triggering those guard conditions on subsequent waits, so long as the work associated with them has not been completed.

[wjwwood]

CI:

• Linux
• Linux-aarch64
• Windows

[wjwwood]

Force pushed to fix DCO.

[jmachowinski]

Uhm, I'm confused, is allocator_memory_strategy.hpp still used ? I thought it was removed with the wait_set patch.

[wjwwood]

Uhm, I'm confused, is allocator_memory_strategy.hpp still used ? I thought it was removed with the wait_set patch.

It's on rolling:

https://github.com/ros2/rclcpp/blob/rolling/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp

I don't see that it was removed in the executor update: https://github.com/ros2/rclcpp/pull/2142/files

Though maybe it should have been removed. @mjcarroll was that an oversight?

[wjwwood]

Rerunning Windows CI after a small fix:

[mjcarroll]

Though maybe it should have been removed. @mjcarroll was that an oversight?

It is unused in the executor implementation, but part of public API. Do we need to tick-tock it or is it safe to remove in one version because it's an implementation detail?

Happy to do either in a follow-up.

[wjwwood]

It's tricky, I'd like to say we could just remove it because it was kind of an internal API, but it's probably safer to deprecate it first.

[wjwwood]

Fresh set of full CI with the Windows fix:

• Linux
• Linux-aarch64
• Windows
  • Re-run:

[wjwwood]

I believe the test failure on Windows was a pre-existing flake, and wasn't caused or made worse by this pr. Subsequent runs on CI did not fail.

[jmachowinski]

agreed, lgtm