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

Fix bond handling during nodelet unloading #51

Merged
merged 4 commits into from Nov 1, 2016

Conversation

Projects
None yet
2 participants
@dseifert
Copy link
Contributor

commented Oct 20, 2016

This fixes points 2 and 3 of #50 and introduces two changes:

  1. The callback for broken bonds is reset when unloading, to avoid calling unload() again.
  2. nodelet load uses an ASyncSpinner in order to handle bond communication during shutdown.
@mikaelarguedas

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

nice catch! tested locally and it works just fine, all nodelets are unloaded properly.
Would it be possible to provide an automated test with numerous nodelet loads and confirming that they are all unloaded properly?

Thanks for the contribution!

@dseifert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

What is the definition of a nodelet being unloaded properly?

Daniel Seifert added some commits Oct 30, 2016

Daniel Seifert
disable callback for broken bond when we are breaking it
This avoids the nodelet::LoaderROS::unload() method to be called
twice for the same nodelet, causing an error output.
Daniel Seifert
use AsyncSpinner for nodelet load in order for the shutdown procedure…
… to work

During shutdown, the bonds still need to communicate their status in order
for the nodelet to properly/cleanly/quickly unload. This requires the node
to spin.

@dseifert dseifert force-pushed the dseifert:fix-50 branch from 9fe8694 to 9cb34a5 Oct 30, 2016

@dseifert

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2016

@mikaelarguedas I added tests for my changes. I don't think it is necessary to add a test for multiple nodelets for these, as the issues are reproducable with single nodelets, too (though "success" for the proper bond handling is a bit hard to check for right now, basically it is not getting a certain DEBUG output).

@dseifert dseifert referenced this pull request Oct 30, 2016

Closed

Nodelet unloading broken #50

@mikaelarguedas
Copy link
Contributor

left a comment

@dseifert Thanks for adding the tests, multiple nodelets unloading was not tested before and this PR definitely improves of the unloading behaviour regardless of the number of nodelets. So I'm in favor of merging it.
Thanks for the contribution

@mikaelarguedas mikaelarguedas merged commit 6dcff94 into ros:indigo-devel Nov 1, 2016

3 checks passed

Ipr__nodelet_core__ubuntu_trusty_amd64 Build finished.
Details
Jpr__nodelet_core__ubuntu_trusty_amd64 Build finished.
Details
Kpr__nodelet_core__ubuntu_xenial_amd64 Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.