Ensure waitables handle guard condition retriggering

[wjwwood]

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.

This is still a WIP. I'll add more cross references and details here in the future.

[jmachowinski]

lgtm, execpt for the dummy stuff ;-)

[mjcarroll]

@wjwwood I went through and cleaned out the dummy functions here

[jmachowinski]

@wjwwood Looking at you const changes to the execute API, it occurred to me, that it might be way better to use a std::unique_ptr here.

I got two motivations for this proposal:

• Performance.
  Shared_ptr are relative expensive to copy around, as they trigger a cache lock. And we are in a hot code path, that is executed with a real high frequency.
• Cleaner semantics.
  The waitable should drop the ownership of the taken data, and should have no way of changing it, after the take_data.

[mjcarroll]

I especially agree on the clearer semantics in this case.

[wjwwood]

I will look into the feasibility of using unique_ptr, but I will point out that the performance shouldn't be an issue since we're not copying the shared ptr in the interface I'm touching, but by taking a const-ref to shared_ptr we allow for the function to copy it if needed.

Also for semantics, const-ref to shared_ptr implies copy is possible but not guaranteed (see: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-sharedptrparam-const), so the semantics are clear enough to me at least, but maybe unique_ptr is actually sufficient (which would imply taking ownership as you all said: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-uniqueptrparam)

[wjwwood]

What was there before, non-const-ref to shared_ptr, is definitely not what we want to convey in the APIs semantics, i.e. that it might reset/reseat the given pointer (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-sharedptrparam).

[jmachowinski]

I will point out that the performance shouldn't be an issue since we're not copying the shared ptr in the interface I'm touching

The data pointer needs to be copied at least one time into the AnyExecutable.

[mjcarroll]

@mjcarroll to run CI on this.

[mjcarroll]

I think that this is actually ready for review. I'm going to run CI to at least see where we are at on it.

[mjcarroll]

• Linux
• Linux-aarch64
• Windows

[wjwwood]

I retargeted this onto #2142 temporarily until it is merged, then I'll retarget it to rolling.

[wjwwood]

I'd like to merge #2467 before pushing this one for review.

[wjwwood]

Now that #2467 is merged, I need to fix this pr up, which I will do asap.

[jmachowinski]

@wjwwood I rebase this on head an cleaned it up a bit :
https://github.com/jmachowinski/rclcpp/tree/ensure_waitables_handle_guard_condition_retriggering

Note, I removed the SubscriptionIntraProcess::add_to_wait_set as its already handled in the base class.

[wjwwood]

Thanks, but I don't see any changes yet. Still has conflicts, did you push? Either way I was already working on the fix up, sorry for the duplication of effort.

Edit: I see you put it on a different branch, thanks. I'll compare what I had been working on.

[wjwwood]

Trying to close and reopen the pr to fix the processing of new commits...

[wjwwood]

It won't let me reopen it because the current target branch has been deleted, and I cannot edit it to change the target branch while closed, so I made a new pr: #2483