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.

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

Document issue with shutdown #283

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Conversation

Yadunund
Copy link
Member

Address #170.

README.md Outdated
often leads to the above panic.

The recommendation is to ensure the `Context` is shutdown before a program terminates.
For example, when using `rclcpp`, ensure `rclcpp::shutdown()` is invoked the program exits.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to be careful with this advice.

In particular, if you are writing composable nodes, you basically never want to call rclcpp::shutdown. That's because rclcpp::shutdown is a global operation, and will shut down your node and all other nodes that happen to be composed with your node at the same time. That's probably not what is intended.

The composable container should call rclcpp::shutdown when it is going down, but that is a different thing.

Overall, I'm not saying that you are wrong, but there is subtle distinction here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't composable nodes be created as shared libraries without a main function and hence lack the opportunity to invoke rclcpp::init() and rclcpp::shutdown()? Only executables that implement compile-time composition would include these statements and the advise above should be valid in this scenario?

The default main impl for composable nodes includes an rclcpp::shutdown() https://github.com/ros2/rclcpp/blob/a78d0cbd33b8fe0b4db25c04f7e10017bfca6061/rclcpp_components/src/node_main.cpp.in#L78

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to massage the wording in any other way!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't composable nodes be created as shared libraries without a main function and hence lack the opportunity to invoke rclcpp::init() and rclcpp::shutdown()?

I mean, they shouldn't call it, but that doesn't mean they won't. rclcpp::shutdown, in particular, is often (erroneously) called when a node is done with its work; even our demos are guilty of this: https://github.com/ros2/demos/blob/0f2afe53be38b71c01d43f0900e18187d2e36082/demo_nodes_cpp/src/services/add_two_ints_client_async.cpp#L65

I'm going to suggest some wording to point out that composable nodes shouldn't generally do this.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Yadunund and others added 3 commits October 1, 2024 11:25
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
@Yadunund Yadunund merged commit cf8e8a2 into rolling Oct 1, 2024
8 checks passed
@Yadunund Yadunund deleted the yadu/document_shutdown_issue branch October 1, 2024 18:34
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.

2 participants