Store graph listener inside the context instead of the node graph

[skyegalaxy]

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]

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

[jmachowinski]

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

[skyegalaxy]

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]

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]

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.

[skyegalaxy]

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]

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

[skyegalaxy]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ahcorde]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@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]

@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!)

[skyegalaxy]

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

[fujitatomoya]

@Mergifyio backport kilted jazzy

[mergify]

backport kilted jazzy

✅ Backports have been created

• #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