Store graph listener inside the context instead of the node graph (backport #2952)#2966
Closed
mergify[bot] wants to merge 1 commit intojazzyfrom
Closed
Store graph listener inside the context instead of the node graph (backport #2952)#2966mergify[bot] wants to merge 1 commit intojazzyfrom
mergify[bot] wants to merge 1 commit intojazzyfrom
Conversation
Contributor
Author
|
Cherry-pick of 715a983 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
ahcorde
requested changes
Oct 9, 2025
Contributor
ahcorde
left a comment
There was a problem hiding this comment.
@skyegalaxy do you mind to fix the conflicts ?
0759325 to
7106305
Compare
) * 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>
7106305 to
4303518
Compare
Member
|
Pulls: #2966
|
Member
|
ahcorde
previously approved these changes
Oct 14, 2025
Collaborator
|
This is ABI breaking and may not be backported |
Collaborator
fujitatomoya
left a comment
There was a problem hiding this comment.
@jmachowinski thanks for letting me know that 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a lock order inversion bug detected by Thread Sanitizer between
GraphListener::shutdown_mutex_andContext::on_shutdown_callbacks_mutex_, by storing and instantiating theGraphListenerinside of theContextinstead ofNodeGraph. 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 existingis_shutdown()method inGraphListenerfor 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
jazzyandrolling.This is an automatic backport of pull request #2952 done by Mergify.