Skip to content

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Aug 12, 2025

This PR fixes a subtle bug that could cause subscriptions, clients, and services to stall out where new messages fail to wake up the wait set because the subscription, client, or service itself has not been added to the wait set yet.

The fix itself is very simple:

pub(crate) fn add_to_wait_set(&self, waitable: Waitable) {
    self.channel.add_to_wait_set(waitable);
    let _ = self.wakeup_wait_set.trigger(); // <--- We were forgetting to do this
}

This was an oversight that was missed in the original PR that introduced async execution. The result was:

  • We send off an instruction to add a new waitable to the wait set
  • The wait set is never told to wake up, so it keeps waiting on its current set of primitives
  • Since the wait set doesn't wake up, the new waitable never gets added to it
  • It becomes a matter of luck whether something currently in the wait set ever wakes it up so that the wait set can notice that a new thing has been added

And so the fix is simply to explicitly trigger the wait set to wake up whenever we add a new waitable to it.

I've also added a test that will fail (at least most of the time, it's a bit of a race condition) if it gets ported to main and run without the fix to add_to_wait_set.

As a bonus, I've added a Publisher::notify_on_subscriber_ready method since it was needed for the new test. Almost all of the changed lines of code are actually related to the new test.

mxgrey added 6 commits August 12, 2025 22:36
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mxgrey LGTM, thanks!

@esteve esteve merged commit 38a6a92 into ros2-rust:main Aug 13, 2025
9 checks passed
@mxgrey mxgrey deleted the wake_up_wait_set branch August 13, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants