From 275b1c540dadd3ea86d975fc6756d9f669e146fd Mon Sep 17 00:00:00 2001 From: Skyler Medeiros Date: Fri, 10 Jul 2026 11:44:00 -0700 Subject: [PATCH 1/3] add a regression test to component manager Signed-off-by: Skyler Medeiros --- .../test/test_component_manager_api.cpp | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/rclcpp_components/test/test_component_manager_api.cpp b/rclcpp_components/test/test_component_manager_api.cpp index d87c89ea04..5057a51cbf 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,33 @@ TEST_F(TestComponentManager, components_api) test_components_api(true); } } + +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 + exec->shutdown(); + + // ComponentManager may remove the same node again + EXPECT_NO_THROW(manager.reset()); +} From c30865515bbbc186ac3ce61b8528f3b1d694c696 Mon Sep 17 00:00:00 2001 From: Skyler Medeiros Date: Fri, 10 Jul 2026 12:31:55 -0700 Subject: [PATCH 2/3] test fixture for component manager double shutdown Signed-off-by: Skyler Medeiros --- .../test/test_component_manager_api.cpp | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/rclcpp_components/test/test_component_manager_api.cpp b/rclcpp_components/test/test_component_manager_api.cpp index 5057a51cbf..4c4a8b5af5 100644 --- a/rclcpp_components/test/test_component_manager_api.cpp +++ b/rclcpp_components/test/test_component_manager_api.cpp @@ -374,10 +374,29 @@ TEST_F(TestComponentManager, components_api) } } +// 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 manager = std::make_shared(exec); auto client_node = rclcpp::Node::make_shared("test_component_manager_3186"); exec->add_node(manager); @@ -398,7 +417,7 @@ TEST_F(TestComponentManager, no_throw_remove_node_twice_on_shutdown) ASSERT_TRUE(future.get()->success); // the executor removes all of its nodes on shutdown - exec->shutdown(); + manager->remove_all_nodes_from_executor(); // ComponentManager may remove the same node again EXPECT_NO_THROW(manager.reset()); From cdc1c9436ddcb1829765552f1ef50ad06b835b90 Mon Sep 17 00:00:00 2001 From: Skyler Medeiros Date: Mon, 13 Jul 2026 14:02:48 -0700 Subject: [PATCH 3/3] Check in ComponentManager dtor if node is associated with an executor Signed-off-by: Skyler Medeiros --- rclcpp_components/src/component_manager.cpp | 5 ++++- rclcpp_components/test/test_component_manager_api.cpp | 2 -- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/rclcpp_components/src/component_manager.cpp b/rclcpp_components/src/component_manager.cpp index f9793dc307..ab9bece121 100644 --- a/rclcpp_components/src/component_manager.cpp +++ b/rclcpp_components/src/component_manager.cpp @@ -83,7 +83,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 4c4a8b5af5..a8d1b4e768 100644 --- a/rclcpp_components/test/test_component_manager_api.cpp +++ b/rclcpp_components/test/test_component_manager_api.cpp @@ -418,7 +418,5 @@ TEST_F(TestComponentManager, no_throw_remove_node_twice_on_shutdown) // the executor removes all of its nodes on shutdown manager->remove_all_nodes_from_executor(); - - // ComponentManager may remove the same node again EXPECT_NO_THROW(manager.reset()); }