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

Test fix: don't keep reference to the controller in the test when it should be destroyed in the controller manager #883

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

sgmurray
Copy link
Contributor

@sgmurray sgmurray commented Jan 2, 2023

This fixes #827

@sgmurray
Copy link
Contributor Author

sgmurray commented Jan 2, 2023

The following error message occurs when a node is created with the same name as a node that already exists.

`[WARN]`` [1664625270.031583774] [rcl.logging_rosout]: Publisher already registered for provided node name. If this is due to multiple nodes with the same name then all logs for that logger name will go out over the existing publisher. As soon as any node with that name is destructed it will unregister the publisher, preventing any further logs for that name from being published on the rosout topic.

With regards to this testcase,
auto ctrl_1 = cm_->get_loaded_controllers()[0]; creates a new ControllerSpec object which contains a shared pointer to the controller's ControllerInterfaceBase, Consequently, even when the controller manager unloads the controller. if ctrl_1 is still in scope then then the ControllerInterfaceBase is not destroyed. Because the ControllerInterfaceBase is not destroyed and ControllerInterfaceBase contains a shared pointer to the node, the node is also not destroyed.

@destogl
Copy link
Member

destogl commented Jan 5, 2023

Thanks for doing a deep dive on this!

With regards to this testcase, auto ctrl_1 = cm_->get_loaded_controllers()[0]; creates a new ControllerSpec object which contains a shared pointer to the controller's ControllerInterfaceBase, Consequently, even when the controller manager unloads the controller. if ctrl_1 is still in scope then then the ControllerInterfaceBase is not destroyed. Because the ControllerInterfaceBase is not destroyed and ControllerInterfaceBase contains a shared pointer to the node, the node is also not destroyed.

I understand this explanation, but don't understand what this has to do with this test. Where are we destroying controller, exactly?

Sorry, I see it now at the end. Cool, this looks good, thanks!

destogl
destogl previously approved these changes Jan 5, 2023
controller_manager/test/test_spawner_unspawner.cpp Outdated Show resolved Hide resolved
@destogl destogl changed the title Iss827 Test fix: don't keep reference to the controller in the test when it should be destroyed in the controller manager Jan 5, 2023
@bmagyar
Copy link
Member

bmagyar commented Jan 16, 2023

Any comment on the failing test (the one modified by this PR) ?

@sgmurray
Copy link
Contributor Author

Any comment on the failing test (the one modified by this PR) ?

I believe that the failing test is TestLoadController.spawner_test_type_in_arg , the test modified in this PR is TestLoadController.spawner_test_type_in_param.

@bmagyar
Copy link
Member

bmagyar commented Jan 18, 2023

The tests prove your point, thanks! :D

@bmagyar bmagyar merged commit 70a156e into ros-controls:master Jan 18, 2023
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.

[CM-Tests] Spawner/Unspawner tests are failing because unloading of controller is not correct.
3 participants