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

Bad order of node.destroy_node() and rclpy.try_shutdown() in demos/demo_nodes_py/demo_nodes_py/topics/listener.py #553

Closed
hodnajit opened this issue Jan 21, 2022 · 5 comments · Fixed by #555
Labels
more-information-needed Further information is required

Comments

@hodnajit
Copy link

I found out, that in this example, there is bad order in

node.destroy_node() 
and rclpy.try_shutdown()

in demos/demo_nodes_py/demo_nodes_py/topics/listener.py

I think, it should be fixed :)

@clalancette
Copy link
Contributor

Looking at the code in https://github.com/ros2/demos/blob/master/demo_nodes_py/demo_nodes_py/topics/listener.py, it looks like we do:

  1. init
  2. create node
  3. use node
  4. destroy node
  5. shutdown

That seems to be the correct order to me. Can you explain more about why you think it is the incorrect order?

@clalancette clalancette added the more-information-needed Further information is required label Jan 21, 2022
@hodnajit
Copy link
Author

Thanks for answering that fast :)
I am confused, that in https://github.com/ros2/demos/blob/master/demo_nodes_py/demo_nodes_py/topics/talker.py

The order of destroying node and shutting rcl is different

rclpy.try_shutdown()
node.destroy_node()

What is the correct way?

@clalancette
Copy link
Contributor

Ah, good catch. In general, it is best to destroy resources in the reverse order that you allocated them. In this case, the talker is wrong. Would you mind submitting a pull request reversing the order in the talker?

@clalancette
Copy link
Contributor

#555 should fix this.

@hodnajit
Copy link
Author

Thank you very much :) Great work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants