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

rtt_roscomm cleaning up registered topics #61

Closed
guihomework opened this issue Jun 7, 2016 · 4 comments
Closed

rtt_roscomm cleaning up registered topics #61

guihomework opened this issue Jun 7, 2016 · 4 comments
Milestone

Comments

@guihomework
Copy link
Contributor

Similar to #59 it seems that the registered subscribers (maybe also publishers) are not correctly shutdown. This leads to a topic still showing the rtt_environment as a subscriber to the topic after unloading the component that had a stream to ros.

I analyzed a little and it appears that the Channel for a publisher gets its destructor called because it is shared pointer https://github.com/orocos/rtt_ros_integration/blob/indigo-devel/rtt_roscomm/include/rtt_roscomm/rtt_rostopic_ros_msg_transporter.hpp#L247

I think one should also call shutdown on the ros_pub in the destructor after it is removed from the instance https://github.com/orocos/rtt_ros_integration/blob/indigo-devel/rtt_roscomm/include/rtt_roscomm/rtt_rostopic_ros_msg_transporter.hpp#L129

the same for the ros_sub in its destructor. I started to implement that, but the biggest problem I faced is that the destructor of the Channel for a subscriber was never called https://github.com/orocos/rtt_ros_integration/blob/indigo-devel/rtt_roscomm/include/rtt_roscomm/rtt_rostopic_ros_msg_transporter.hpp#L220
due to the msg transport creating a standard pointer and not a shared_ptr for it https://github.com/orocos/rtt_ros_integration/blob/indigo-devel/rtt_roscomm/include/rtt_roscomm/rtt_rostopic_ros_msg_transporter.hpp#L258 Not sure if the fact that the return type is a shared_ptr counts, because I added prints in the destructor and they are not printed.

I now know this is a header that has a larger implication and I struggled to understand that I had to rebuild the typekits for the ros message I wanted to test this cleanup for. However, even after making it a shared pointer it did still not enter the destructor.

What am I missing ?

@meyerj meyerj added this to the 2.9 milestone Jul 12, 2016
@meyerj
Copy link
Member

meyerj commented Jul 12, 2016

the same for the ros_sub in its destructor. I started to implement that, but the biggest problem I faced is that the destructor of the Channel for a subscriber was never called rtt_rostopic_ros_msg_transporter.hpp#L220 due to the msg transport creating a standard pointer and not a shared_ptr for it rtt_rostopic_ros_msg_transporter.hpp#L258 Not sure if the fact that the return type is a shared_ptr counts, because I added prints in the destructor and they are not printed.

Indeed because the pointer is returned as a shared pointer, the channel including the RosSubChannelElement<T> instance should be destructed after a disconnect.

I will investigate this during the next days. What RTT version did you use for your tests? It could be a bug during the cleanup of stream connections.

@guihomework
Copy link
Contributor Author

Thanks for addressing this issue. I was running the ros package ros-indigo-rtt (2.8.2-0trusty-20160321-1) and built the indigo-devel branch at that commit (18c59db).

To check if the topic was unregistered, I used rostopic info command line tools, and was expecting the rtt_environment (running from rttlua-gnulinux) to disappear from the subscribers/publishers listed by the tool, which did not work.

@meyerj
Copy link
Member

meyerj commented Jul 12, 2016

I found that rtt_rostopic_ros_msg_transporter.hpp:217 is responsible for increasing the reference count of the RosSubChannelElement<T> instance and therefore it will never be deleted. At least in RTT 2.9 connection cleanup should work properly without this line and I verified by adding a check to rtt_roscomm_tests that the publishers and subscribers are properly shutdown after a port disconnect. I will push the patch tomorrow, after I tested it with toolchain 2.8.

@smits, do you remember why this line was necessary back in 2010?

meyerj added a commit to meyerj/rtt_ros_integration that referenced this issue Jul 12, 2016
…scriber shutdown (fix orocos#61)

The transport_test in package rtt_roscomm_tests verifies that both, the publisher and the subscriber,
have been deregistered in roscpp after the port disconnect.
@meyerj meyerj closed this as completed in 36ac696 Jul 20, 2016
meyerj added a commit that referenced this issue Jul 20, 2016
@guihomework
Copy link
Contributor Author

Dear @meyerj the fix you provided apparently does not suffice. The RosSubChannelElement is not a shared pointer (https://github.com/orocos/rtt_ros_integration/blob/indigo-devel/rtt_roscomm/include/rtt_roscomm/rtt_rostopic_ros_msg_transporter.hpp#L257)
If converted to a shared ptr then the subscriber is correctly destroyed and does not appear in the list anymore
I have a PR for it coming.

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

No branches or pull requests

2 participants