Skip to content

Fix race condition on shutdown in try_discover_source()#143

Merged
MichaelOrlov merged 2 commits intoros-tooling:rollingfrom
MarqRazz:fix_shutdown
Apr 21, 2026
Merged

Fix race condition on shutdown in try_discover_source()#143
MichaelOrlov merged 2 commits intoros-tooling:rollingfrom
MarqRazz:fix_shutdown

Conversation

@MarqRazz
Copy link
Copy Markdown
Contributor

@MarqRazz MarqRazz commented Mar 9, 2026

In ros2_control we have been having issues with topic_tools/relay not playing nice with SIGINT and failing tests. RelayNode creates a discovery_timer_ that fires every 100ms and calls make_subscribe_unsubscribe_decisions() → try_discover_source() → get_publishers_info_by_topic(). When SIGINT is received, rclcpp::shutdown() invalidates the context, but the timer can still fire and call get_publishers_info_by_topic on the now-invalid context, throwing an uncaught exception.

This PR adds a guard to check for the node shutting down and returns early so the exception is not thrown.

Signed-off-by: MarqRazz <marq.razz@gmail.com>
Copy link
Copy Markdown
Member

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@christophfroehlich
Copy link
Copy Markdown
Contributor

@MichaelOrlov Is this ready to be merged? Thank you!

@MichaelOrlov
Copy link
Copy Markdown
Member

@christophfroehlich Thank you for pinging me here. I overlooked when CI finished and forgot to merge this PR.

@MichaelOrlov MichaelOrlov merged commit f40f5be into ros-tooling:rolling Apr 21, 2026
13 checks passed
@christophfroehlich
Copy link
Copy Markdown
Contributor

Thanks for merging. Can I ask you for a release of the package? It'll hopefully fix some of our flaky tests ;)

@MarqRazz MarqRazz deleted the fix_shutdown branch April 22, 2026 01:09
@MichaelOrlov
Copy link
Copy Markdown
Member

@christophfroehlich
Copy link
Copy Markdown
Contributor

thank you!

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.

3 participants