Fix rolling smoke tests crashing #309
Conversation
achim-k
changed the title
| const size_t numThreads = 2; | ||
| auto executor = | ||
| rclcpp::executors::MultiThreadedExecutor::make_shared(rclcpp::ExecutorOptions{}, numThreads); | ||
| auto executor = rclcpp::executors::SingleThreadedExecutor::make_shared(); |
There was a problem hiding this comment.
why change to single threaded?
There was a problem hiding this comment.
no good reason actually, just noticed that it also works with a single threaded executor so I left it
| @@ -759,6 +757,7 @@ int main(int argc, char** argv) { | |||
| const auto testResult = RUN_ALL_TESTS(); | |||
| executor->cancel(); | |||
| spinnerThread.join(); | |||
| executor.reset(); | |||
There was a problem hiding this comment.
why is this necessary? this is just a shared_ptr right? so even though we already called cancel(), it is problematic to have the object's lifetime last beyond the shutdown() call? Probably could use a comment here because this line seems fairly useless otherwise.
If the lifetime matters so much, can we use make_unique instead of make_shared?
There was a problem hiding this comment.
I think the component manager was still holding on to it which caused issues. I have simplified this now quite a lot and am using the node directly instead of loading it via the component manager.
Changelog
Fix rolling test crashes
Docs
None
Description
Fixes the rolling smoke tests crashing due to incorrect executor cleanup. In the end, the only thing missing was a
executor.reset();at the end ofmain. However I took the opportunity to clean up a little.Fixes #304