Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure waitables handle guard condition retriggering #2483

Merged
merged 8 commits into from
Apr 8, 2024
Merged

Ensure waitables handle guard condition retriggering #2483

merged 8 commits into from
Apr 8, 2024

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Apr 4, 2024

Re-opened version of #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 wjwwood self-assigned this Apr 4, 2024
@wjwwood wjwwood requested review from ivanpauno and hidmic as code owners April 4, 2024 00:25
@wjwwood wjwwood mentioned this pull request Apr 4, 2024
Closed
@wjwwood
Copy link
Member Author

wjwwood commented Apr 4, 2024

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

wjwwood and others added 7 commits April 3, 2024 17:53
@wjwwood
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
8a470c3 
…or (#2109)"

This reverts commit 232262c.

Signed-off-by: William Woodall <[email protected]>
@wjwwood
wip: updating waitables to ensure things stay ready when needed, debu…
1db423c 
…gging code still there

Signed-off-by: William Woodall <[email protected]>
@mjcarroll @wjwwood
Remove dummy sentinel
3dc3796 
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: William Woodall <[email protected]>
@wjwwood
add tests for waitables, fix const-ness in some of the APIs
d101035 
Signed-off-by: William Woodall <[email protected]>
@wjwwood
fixup revert mistake
0ee9f72 
Signed-off-by: William Woodall <[email protected]>
@wjwwood
fixup due to changes on rolling
e325fe8 
Signed-off-by: William Woodall <[email protected]>
@wjwwood
style
327cb12 
Signed-off-by: William Woodall <[email protected]>
@wjwwood wjwwood force-pushed the wjwwood/ensure_waitables_handle_guard_condition_retriggering branch from f49e86f to 327cb12 Compare April 4, 2024 00:54
@wjwwood
Copy link
Member Author

wjwwood commented Apr 4, 2024

Force pushed to fix DCO.

@jmachowinski
Copy link
Contributor

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

@wjwwood
Copy link
Member Author

wjwwood commented Apr 4, 2024

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
Copy link
Member Author

wjwwood commented Apr 4, 2024

Rerunning Windows CI after a small fix: Build Status

@mjcarroll
Copy link
Member

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
Copy link
Member Author

wjwwood commented Apr 4, 2024

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
Copy link
Member Author

wjwwood commented Apr 4, 2024 •
edited
Loading

Fresh set of full CI with the Windows fix:

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

@wjwwood
Copy link
Member Author

wjwwood commented Apr 8, 2024

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
Copy link
Contributor

agreed, lgtm

@mjcarroll mjcarroll merged commit bfa7efa into rolling Apr 8, 2024
3 checks passed
@mjcarroll mjcarroll deleted the wjwwood/ensure_waitables_handle_guard_condition_retriggering branch April 8, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette left review comments

@mjcarroll mjcarroll mjcarroll approved these changes

@jmachowinski jmachowinski jmachowinski approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

Assignees

@wjwwood wjwwood

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wjwwood @jmachowinski @mjcarroll @clalancette