Store graph listener inside the context instead of the node graph by skyegalaxy · Pull Request #2952 · ros2/rclcpp

skyegalaxy · 2025-09-11T21:52:19Z

Description

This PR fixes a lock order inversion bug detected by Thread Sanitizer between GraphListener::shutdown_mutex_ and Context::on_shutdown_callbacks_mutex_, by storing and instantiating the GraphListener inside of the Context instead of NodeGraph. With this change, we avoid locking the context weak pointer and inverting the order of lock acquisition on startup and shutdown.

Usage of is_shutdown_->load() was also replaced with the existing is_shutdown() method in GraphListener for consistency.

Fixes # (2946)

Is this user-facing behavior change?

no

Did you use Generative AI?

no

Additional Information

This code appears to have remained unchanged for a long time, and I was able to reproduce this tsan error on iron, jazzy and rolling with this example. Tsan identified a lock order inversion as well as one potential data race in other parts of the code. With this change, the lock order inversion warning is gone, no additional tsan warnings were introduced, and all unit tests passed locally when these changes were built against both jazzy and rolling.

skyegalaxy · 2025-09-11T22:53:50Z

Seems that the one failing test rclcpp.TestExecutor.spin_all_fail_wait_set_clear is known to be flaky, from examining previous PRs.

rclcpp/src/rclcpp/graph_listener.cpp

jmachowinski · 2025-09-12T13:44:33Z

Why the new interface, would it not be sufficient to move the lock down in start_if_not_started() ?

skyegalaxy · 2025-09-12T16:04:26Z

Why the new interface, would it not be sufficient to move the lock down in start_if_not_started() ?

My original thinking was that the behavior I factored out seemed like it could be its own functionality and that start_if_not_started felt like it was doing a bit too much compared to its name, but at the end of the day I really just wanna fix the tsan issue and not make this a drastic interface change.

If others agree that the check for is_shutdown_ ought to be okay outside of shutdown_lock_ because it's an atomic_bool, I can just move the lock down

jmachowinski · 2025-09-15T08:15:32Z

I guess the shutdown handler needs to be registered after the start. If not, it could be called (In the super race from hell) before the listener thread is started and you'll end up in a weird state.
Hm looking at this more, I guess we now have a race going on in this special case after the patch. Moving the registration down creates another race (lost shutdown), so we need a better solution...

skyegalaxy · 2025-09-19T17:05:38Z

ok, after speaking with @wjwwood at today's client library WG meeting, I think we've got a solid idea for how to move forward. Rather than having the GraphListener consume a pointer to the context that it then adds a shutdown callback to while holding onto the shutdown mutex, we proposed refactoring the code so that the GraphListener is created within the Context. The Context would lock to prevent multiple GraphListener instantiations, create the GraphListener, register its shutdown hooks, and release the lock. Whereas the GraphListener shutdown callback would acquire its shutdown lock to prevent parallel shutdowns.

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

skyegalaxy · 2025-10-03T14:48:00Z

Along with the known flaky test, the only other failing test in CI appears to be another extremely rare flake. from my clean rolling workspace:

smedeiros@1395239c7f9e:~/SourceCode/rolling-ws-2$ for run in {1..5000}; do ./build/rclcpp/test/rclcpp/test_executors_busy_waiting; done | grep FAILED
[  FAILED  ] TestBusyWaiting/EventsExecutor.test_spin, where TypeParam = rclcpp::experimental::executors::EventsExecutor (10017 ms)
[  FAILED  ] 1 test, listed below:
[  FAILED  ] TestBusyWaiting/EventsExecutor.test_spin, where TypeParam = rclcpp::experimental::executors::EventsExecutor
 1 FAILED TEST
smedeiros@1395239c7f9e:~/SourceCode/rolling-ws-2$

skyegalaxy · 2025-10-03T20:29:20Z

Pulls: #2952
Gist: https://gist.githubusercontent.com/skyegalaxy/68fa478b26b26da11996d4a251a95454/raw/f4814cd9de16ce06f1fbfc30d34b63bde90ffa6c/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17194

Linux
Linux-aarch64
Linux-rhel
Windows

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

skyegalaxy · 2025-10-03T23:34:14Z

Linux
Linux-aarch64
Linux-rhel
Windows

ahcorde · 2025-10-06T08:52:12Z

Linux
Linux-aarch64
Linux-rhel
Windows

rclcpp/src/rclcpp/context.cpp

fujitatomoya · 2025-10-09T00:49:55Z

@skyegalaxy thanks for fixing this up. lgtm with just a nit, which is just comment block. i do not think we need to re-run the CI after the comment fix.

skyegalaxy · 2025-10-09T00:53:18Z

@skyegalaxy thanks for fixing this up. lgtm with just a nit, which is just comment block. i do not think we need to re-run the CI after the comment fix.

Done! 0cbaecf
(looks like the original DCO failure was because I used github's "apply suggestion" feature and it signed off with my personal email instead of my work email!)

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

skyegalaxy · 2025-10-09T01:17:23Z

@fujitatomoya thanks for reviewing and merging my PR! Since we caught this in Jazzy, Is this worth backporting to older distros as well?

fujitatomoya · 2025-10-09T01:19:27Z

@Mergifyio backport kilted jazzy

mergify · 2025-10-09T01:19:38Z

backport kilted jazzy

✅ Backports have been created

Details

#2965 Store graph listener inside the context instead of the node graph (backport #2952) has been created for branch kilted but encountered conflicts
#2966 Store graph listener inside the context instead of the node graph (backport #2952) has been created for branch jazzy but encountered conflicts

) * factor shutdown hook registration into its own method Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * run ament_uncrustify Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * consistent use of is_shutdown in graph-listener Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * this doesn't actually need to be a separate function Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * whitespace Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * fix typo Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * move graph listener into the context Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * make cpplint happy Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * Update rclcpp/src/rclcpp/context.cpp Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> --------- Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> (cherry picked from commit 715a983) # Conflicts: # rclcpp/src/rclcpp/context.cpp

…s2#2952) * factor shutdown hook registration into its own method Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * run ament_uncrustify Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * consistent use of is_shutdown in graph-listener Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * this doesn't actually need to be a separate function Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * whitespace Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * fix typo Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * move graph listener into the context Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * make cpplint happy Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * Update rclcpp/src/rclcpp/context.cpp Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> --------- Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

) * factor shutdown hook registration into its own method Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * run ament_uncrustify Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * consistent use of is_shutdown in graph-listener Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * this doesn't actually need to be a separate function Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * whitespace Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * fix typo Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * move graph listener into the context Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * make cpplint happy Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> * Update rclcpp/src/rclcpp/context.cpp Signed-off-by: Skyler Medeiros <smedeiros@irobot.com> --------- Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

skyegalaxy changed the title ~~Factor out shutdown hook registration from start_if_not_started in GraphListener~~ Factor out shutdown hook registration from start_if_not_started in GraphListener Sep 11, 2025

skyegalaxy marked this pull request as ready for review September 11, 2025 22:54

fujitatomoya reviewed Sep 12, 2025

View reviewed changes

rclcpp/src/rclcpp/graph_listener.cpp Outdated Show resolved Hide resolved

skyegalaxy changed the title ~~Factor out shutdown hook registration from start_if_not_started in GraphListener~~ Move shutdown hook registration outside of shutdown_mutex_ in GraphListener::start_if_not_started Sep 12, 2025

kscottz assigned skyegalaxy Sep 25, 2025

skyegalaxy added 7 commits September 30, 2025 11:43

factor shutdown hook registration into its own method

659c610

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

run ament_uncrustify

1a86edd

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

consistent use of is_shutdown in graph-listener

8e82c69

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

this doesn't actually need to be a separate function

b0f5e4e

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

whitespace

96a97f5

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

fix typo

f0d2357

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

move graph listener into the context

0440c31

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

skyegalaxy force-pushed the graph-listener-lock-order-fixes branch from 06b4b29 to 0440c31 Compare October 2, 2025 23:34

skyegalaxy changed the title ~~Move shutdown hook registration outside of shutdown_mutex_ in GraphListener::start_if_not_started~~ Store graph listener inside the context instead of the node graph Oct 2, 2025

make cpplint happy

56b4684

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

ahcorde requested a review from fujitatomoya October 8, 2025 10:27

fujitatomoya requested changes Oct 9, 2025

View reviewed changes

rclcpp/src/rclcpp/context.cpp Outdated Show resolved Hide resolved

Update rclcpp/src/rclcpp/context.cpp

0cbaecf

Signed-off-by: Skyler Medeiros <smedeiros@irobot.com>

skyegalaxy force-pushed the graph-listener-lock-order-fixes branch from 7e70733 to 0cbaecf Compare October 9, 2025 01:03

skyegalaxy requested a review from fujitatomoya October 9, 2025 01:05

fujitatomoya approved these changes Oct 9, 2025

View reviewed changes

fujitatomoya merged commit 715a983 into ros2:rolling Oct 9, 2025
2 of 3 checks passed

This was referenced Oct 9, 2025

Store graph listener inside the context instead of the node graph (backport #2952) #2965

Closed

Store graph listener inside the context instead of the node graph (backport #2952) #2966

Closed

mjcarroll mentioned this pull request Dec 3, 2025

🧑‍🌾 Flaky test TestExecutor.spin_all_fail_wait_set_clear in Linux #2926

Closed

Conversation

skyegalaxy commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Is this user-facing behavior change?

Did you use Generative AI?

Additional Information

Uh oh!

skyegalaxy commented Sep 11, 2025

Uh oh!

Uh oh!

jmachowinski commented Sep 12, 2025

Uh oh!

skyegalaxy commented Sep 12, 2025

Uh oh!

jmachowinski commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyegalaxy commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyegalaxy commented Oct 3, 2025

Uh oh!

skyegalaxy commented Oct 3, 2025

Uh oh!

skyegalaxy commented Oct 3, 2025

Uh oh!

ahcorde commented Oct 6, 2025

Uh oh!

Uh oh!

fujitatomoya commented Oct 9, 2025

Uh oh!

skyegalaxy commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

skyegalaxy commented Oct 9, 2025

Uh oh!

fujitatomoya commented Oct 9, 2025

Uh oh!

mergify bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skyegalaxy commented Sep 11, 2025 •

edited

Loading

jmachowinski commented Sep 15, 2025 •

edited

Loading

skyegalaxy commented Sep 19, 2025 •

edited

Loading

skyegalaxy commented Oct 9, 2025 •

edited

Loading

mergify bot commented Oct 9, 2025 •

edited

Loading