From fe7aebe925ce0947e27e29f1c7a4240fe6fa71c1 Mon Sep 17 00:00:00 2001 From: Skyler Medeiros Date: Wed, 15 Jul 2026 14:22:06 -0700 Subject: [PATCH] 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 636c6e08c4..6925d28523 100644 --- a/rclcpp_components/src/component_manager.cpp +++ b/rclcpp_components/src/component_manager.cpp @@ -69,7 +69,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 d87c89ea04..a8d1b4e768 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" @@ -372,3 +373,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()); +}