Cleanup of https://github.com/ros2/rclcpp/pull/2683

[jmachowinski]

We did a quick fix of #2664 in order to get it merged fast in jazzy.

This is the proper fix for the issue reported in #2664

[jmachowinski]

@mjcarroll @wjwwood @fujitatomoya @alsora
While doing the cleanup I found some other issues in the code. We should definitely backport the double adding of the notifier the jazzy....

[jmachowinski]

There is still one issue going on, were I don't know if its a bug in the rmw or not.
The reason why I don't know if it is a bug or not, is that it is not clearly specified what
the behavior is if I add an entity twice to a waitset.

If you add a guard condition twice to a rcl_waitsets, the behavior is odd.
If the guard condition is triggered, only the first entry in the rcl_waitset is set, the second is nullptr.

The problem here, is that this breaks code like this :

  rcl_guard_condition_t * cond = &guard_condition_->get_rcl_guard_condition();

  size_t idx1;
  rcl_wait_set_add_guard_condition(&wait_set, cond, &idx1);

  size_t idx2;
  rcl_wait_set_add_guard_condition(&wait_set, cond, &idx2);

  guard_condition_->trigger();

  rcl_wait(&wait_set, 1);
  
  if(wait_set.guard_conditions[idx1] != nullptr)
  {
    // do something
  }

 // this entry will be a nullptr, even though it should not.
  if(wait_set.guard_conditions[idx2] != nullptr)
  {
    // do something
  }

[clalancette]

If you add a guard condition twice to a rcl_waitsets, the behavior is odd.

Which RMW are you testing with? While the behavior should in theory be consistent across them, they all implement it fairly differently.

[jmachowinski]

@clalancette I used the default, so this should have been fastDDS.
The big question here is, what is the wanted behavior ?
If we all agree, that the second guard condition, should not be null as well, I can come up with a test and patch....

[wjwwood]

This doesn't seem right to me, are we not holding the strong_reference unconditionally? And if not should we not check if the weak one locks?

And it seems like we only use the weak_reference for comparison, then why not just have the strong reference, use it for comparison, and not have the GuardHolder at all?

[jmachowinski]

You are right, if we hold the shared_ptr, the lock of the weak_ptr will always work, and we don't need to do all of this, I will adjust the code.

[wjwwood]

The big question here is, what is the wanted behavior ?
If we all agree, that the second guard condition, should not be null as well, I can come up with a test and patch....

We discussed this in the ROS 2 PMC meeting as well as in the latest client library working group meeting, but for everyone else's benefit the conclusion was that adding the same entity (guard condition, timer, sub, etc...) to a wait set should be an error and we shouldn't require the rmw layer to allow it and leave both non-null. I believe the agreement was to make the check in the functions that add the entities to the wait set, but we may also try to avoid this in the calling code in rclcpp.