From 9989ed7c7b94892791a885444305b883a527dc7f Mon Sep 17 00:00:00 2001 From: Skyler Medeiros Date: Wed, 15 Jul 2026 14:22:06 -0700 Subject: [PATCH 1/2] Check for association with an executor before removing nodes in ComponentManager (#3190) * add a regression test to component manager Signed-off-by: Skyler Medeiros * test fixture for component manager double shutdown Signed-off-by: Skyler Medeiros * Check in ComponentManager dtor if node is associated with an executor Signed-off-by: Skyler Medeiros --------- Signed-off-by: Skyler Medeiros Co-authored-by: Skyler Medeiros (cherry picked from commit d2d94d0357b6602c6a4d457b3eb6ed57106b9427) --- rclcpp_components/src/component_manager.cpp | 5 +- .../test/test_component_manager_api.cpp | 48 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/rclcpp_components/src/component_manager.cpp b/rclcpp_components/src/component_manager.cpp index 16329a4247..507d973000 100644 --- a/rclcpp_components/src/component_manager.cpp +++ b/rclcpp_components/src/component_manager.cpp @@ -70,7 +70,10 @@ ComponentManager::~ComponentManager() RCLCPP_DEBUG(get_logger(), "Removing components from executor"); if (auto exec = executor_.lock()) { for (auto & wrapper : node_wrappers_) { - exec->remove_node(wrapper.second.get_node_base_interface()); + auto node_interface = wrapper.second.get_node_base_interface(); + if (node_interface->get_associated_with_executor_atomic().load()) { + exec->remove_node(node_interface); + } } } } diff --git a/rclcpp_components/test/test_component_manager_api.cpp b/rclcpp_components/test/test_component_manager_api.cpp index 5282e1fb8c..7c21f087b5 100644 --- a/rclcpp_components/test/test_component_manager_api.cpp +++ b/rclcpp_components/test/test_component_manager_api.cpp @@ -22,6 +22,7 @@ #include "composition_interfaces/srv/unload_node.hpp" #include "composition_interfaces/srv/list_nodes.hpp" +#include "rclcpp/executors/events_cbg_executor/events_cbg_executor.hpp" #include "rclcpp_components/component_manager.hpp" #include "rclcpp_components/component_manager_isolated.hpp" @@ -389,3 +390,50 @@ TEST_F(TestComponentManager, components_api) test_components_api(true); } } + +// Test fixture for the component manager which exercises node removal like on +// shutdown. EventsCBGExecutor::shutdown() is protected, so this is to +// reproduce its effect (dropping every node from the executor while the manager +// still tracks them in node_wrappers_) +class DoubleRemoveComponentManager : public rclcpp_components::ComponentManager +{ +public: + using rclcpp_components::ComponentManager::ComponentManager; + + void remove_all_nodes_from_executor() + { + if (auto exec = executor_.lock()) { + for (auto & wrapper : node_wrappers_) { + exec->remove_node(wrapper.second.get_node_base_interface()); + } + } + } +}; + +TEST_F(TestComponentManager, no_throw_remove_node_twice_on_shutdown) +{ + auto exec = std::make_shared(); + auto manager = std::make_shared(exec); + auto client_node = rclcpp::Node::make_shared("test_component_manager_3186"); + + exec->add_node(manager); + exec->add_node(client_node); + + auto composition_client = client_node->create_client( + "/ComponentManager/_container/load_node"); + ASSERT_TRUE(composition_client->wait_for_service(20s)) << "service not available after waiting"; + + // Load a component so the manager tracks a node that is associated with the + // executor (~ComponentManager only removes nodes when node_wrappers_ is + // non-empty). + auto request = std::make_shared(); + request->package_name = "rclcpp_components"; + request->plugin_name = "test_rclcpp_components::TestComponentFoo"; + auto future = composition_client->async_send_request(request); + ASSERT_EQ(exec->spin_until_future_complete(future, 5s), rclcpp::FutureReturnCode::SUCCESS); + ASSERT_TRUE(future.get()->success); + + // the executor removes all of its nodes on shutdown + manager->remove_all_nodes_from_executor(); + EXPECT_NO_THROW(manager.reset()); +} From 2ad64345d78444a5163cfe62bc8536797044e1cc Mon Sep 17 00:00:00 2001 From: Skyler Medeiros Date: Thu, 16 Jul 2026 11:27:16 -0700 Subject: [PATCH 2/2] remove regression test non-applicable in humble Signed-off-by: Skyler Medeiros --- .../test/test_component_manager_api.cpp | 48 ------------------- 1 file changed, 48 deletions(-) diff --git a/rclcpp_components/test/test_component_manager_api.cpp b/rclcpp_components/test/test_component_manager_api.cpp index 7c21f087b5..5282e1fb8c 100644 --- a/rclcpp_components/test/test_component_manager_api.cpp +++ b/rclcpp_components/test/test_component_manager_api.cpp @@ -22,7 +22,6 @@ #include "composition_interfaces/srv/unload_node.hpp" #include "composition_interfaces/srv/list_nodes.hpp" -#include "rclcpp/executors/events_cbg_executor/events_cbg_executor.hpp" #include "rclcpp_components/component_manager.hpp" #include "rclcpp_components/component_manager_isolated.hpp" @@ -390,50 +389,3 @@ TEST_F(TestComponentManager, components_api) test_components_api(true); } } - -// Test fixture for the component manager which exercises node removal like on -// shutdown. EventsCBGExecutor::shutdown() is protected, so this is to -// reproduce its effect (dropping every node from the executor while the manager -// still tracks them in node_wrappers_) -class DoubleRemoveComponentManager : public rclcpp_components::ComponentManager -{ -public: - using rclcpp_components::ComponentManager::ComponentManager; - - void remove_all_nodes_from_executor() - { - if (auto exec = executor_.lock()) { - for (auto & wrapper : node_wrappers_) { - exec->remove_node(wrapper.second.get_node_base_interface()); - } - } - } -}; - -TEST_F(TestComponentManager, no_throw_remove_node_twice_on_shutdown) -{ - auto exec = std::make_shared(); - auto manager = std::make_shared(exec); - auto client_node = rclcpp::Node::make_shared("test_component_manager_3186"); - - exec->add_node(manager); - exec->add_node(client_node); - - auto composition_client = client_node->create_client( - "/ComponentManager/_container/load_node"); - ASSERT_TRUE(composition_client->wait_for_service(20s)) << "service not available after waiting"; - - // Load a component so the manager tracks a node that is associated with the - // executor (~ComponentManager only removes nodes when node_wrappers_ is - // non-empty). - auto request = std::make_shared(); - request->package_name = "rclcpp_components"; - request->plugin_name = "test_rclcpp_components::TestComponentFoo"; - auto future = composition_client->async_send_request(request); - ASSERT_EQ(exec->spin_until_future_complete(future, 5s), rclcpp::FutureReturnCode::SUCCESS); - ASSERT_TRUE(future.get()->success); - - // the executor removes all of its nodes on shutdown - manager->remove_all_nodes_from_executor(); - EXPECT_NO_THROW(manager.reset()); -}