Fix callback group logic in executor

[mjcarroll]

Fixes #2474

CC: @jmachowinski

[jmachowinski]

Is it easy to create a unit-test that would fail without this PR and succeeds now?

Should be straight forward. A custom executor, Add a cbg call collect, then delete the cbg Call get next executable. This failed for me, I got a any executable with an empty cbg

[mjcarroll]

@jmachowinski can you take a look at my test here? I can't get a scenario where the executable (in this case a timer) is valid, while the callback group is not.

In my case, even when resetting the callback group and the node, there are still 2 strong references to the callback group.

[jmachowinski]

And I've found that using a custom waitable that can be explicitly triggered rather than a timer can avoid unintentional race conditions and sleeps

In this special case, the timer is not racy, as we just need it to be ready. Also I would like to keep the timer, as it uncovered a bug causing an endless loop.

[mjcarroll]

@jmachowinski I have incorporated your changes from: https://github.com/cellumation/rclcpp/tree/fix_cbg_condition

They have been squashed and I marekd you with co-authored-by

• Linux
• Linux-aarch64
• Windows

[jmachowinski]

@mjcarroll I picked the test over to https://github.com/ros2/rclcpp/pull/2494

2494 also fixes the tests fails that came up in the CI.

I think we can close this PR in favor for 2494

[wjwwood]

Ok, let's go ahead and close this then. We can reopen it if needed, and I think the feed back was addressed in the new pr as far as I can tell.

For future reference, it's usually better to make prs to other prs rather than replacing them, though sometimes that doesn't work out due to needing to rebase. Or like me you mess up the pr or branch some how and have to close it 🙃.