Fix race condition when creating isolated component container

rclcpp_components/include/rclcpp_components/component_manager_isolated.hpp

@@ -102,7 +102,12 @@ class ComponentManagerIsolated : public rclcpp_components::ComponentManager
     if constexpr (std::is_same_v<ExecutorT, rclcpp::executors::SingleThreadedExecutor>) {
       exec = std::make_shared<ExecutorT>(executor_options_);
     } else {
-      exec = std::make_shared<ExecutorT>(executor_options_, num_threads_);
+      // num_threads_ == 0 means "auto": read the `thread_num` parameter declared by
+      // ComponentManager (defaults to the hardware concurrency).
+      const size_t num_threads = num_threads_ != 0 ?
+        num_threads_ :
+        static_cast<size_t>(this->get_parameter("thread_num").as_int());
+      exec = std::make_shared<ExecutorT>(executor_options_, num_threads);
     }
     exec->add_node(node_wrappers_[node_id].get_node_base_interface());
 

rclcpp_components/src/component_container.cpp

@@ -143,46 +143,48 @@ int main(int argc, char * argv[])
   }
 
   std::shared_ptr<rclcpp::Executor> exec;
-  std::shared_ptr<rclcpp_components::ComponentManager> node =
-    std::make_shared<rclcpp_components::ComponentManager>();
-  const int64_t num_threads = (node->has_parameter("thread_num")) ?
-    node->get_parameter("thread_num").as_int() :
-    std::thread::hardware_concurrency();
-  std::string debug_msg;
+  std::shared_ptr<rclcpp_components::ComponentManager> node;
 
-  if (args.executor_type == ExecutorType::SingleThreaded &&
-    num_threads > 0 &&
-    num_threads != std::thread::hardware_concurrency())
-  {
-    RCUTILS_LOG_WARN_NAMED("component_container",
-      "thread_num is not supported by the SingleThreadedExecutor. Ignoring...");
-  }
+  // Create the manager once, as its final type. The isolated managers resolve the
+  // `thread_num` parameter themselves when a component is loaded (0 == auto).
   if (args.isolated) {
-    // we use the ComponentManager node initially to get the `thread_num` parameter,
-    // but temporarily delete it here before re-assigning it
-    // if running with a ComponentManagerIsolated.
-    // This is to avoid a possible race condition
-    // where the 2 nodes may be briefly alive at the same time,
-    node = nullptr;
-    // The outer executor runs only the container manager's load/unload services.
-    // Each loaded component gets its own dedicated executor of the requested type.
-    exec = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
     switch (args.executor_type) {
       case ExecutorType::MultiThreaded:
         node = std::make_shared<
           rclcpp_components::ComponentManagerIsolated<rclcpp::executors::MultiThreadedExecutor>>(
-          rclcpp::ExecutorOptions(), num_threads);
+          rclcpp::ExecutorOptions(), 0);
         break;
       case ExecutorType::EventsCBG:
         node = std::make_shared<
           rclcpp_components::ComponentManagerIsolated<rclcpp::executors::EventsCBGExecutor>>(
-          rclcpp::ExecutorOptions(), num_threads);
+          rclcpp::ExecutorOptions(), 0);
         break;
       default:
         node = std::make_shared<
           rclcpp_components::ComponentManagerIsolated<rclcpp::executors::SingleThreadedExecutor>>();
         break;
     }
+  } else {
+    node = std::make_shared<rclcpp_components::ComponentManager>();
+  }
+
+  const int64_t num_threads = (node->has_parameter("thread_num")) ?
+    node->get_parameter("thread_num").as_int() :
+    std::thread::hardware_concurrency();
+
+  if (args.executor_type == ExecutorType::SingleThreaded &&
+    num_threads > 0 &&
+    num_threads != std::thread::hardware_concurrency())
+  {
+    RCUTILS_LOG_WARN_NAMED("component_container",
+      "thread_num is not supported by the SingleThreadedExecutor. Ignoring...");
+  }
+
+  std::string debug_msg;
+  if (args.isolated) {
+    // The outer executor runs only the container manager's load/unload services.
+    // Each loaded component gets its own dedicated executor of the requested type.
+    exec = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
 
     debug_msg = "Creating isolated component container with the following per-node settings: "
       "executor_type: " + executor_type_to_string(args.executor_type);

rclcpp_components/test/test_component_manager_api.cpp

@@ -23,6 +23,7 @@
 #include "composition_interfaces/srv/list_nodes.hpp"
 
 #include "rclcpp/executors/events_cbg_executor/events_cbg_executor.hpp"
+#include "rclcpp/executors/multi_threaded_executor.hpp"
 #include "rclcpp_components/component_manager.hpp"
 #include "rclcpp_components/component_manager_isolated.hpp"
 
@@ -420,3 +421,33 @@ TEST_F(TestComponentManager, no_throw_remove_node_twice_on_shutdown)
   manager->remove_all_nodes_from_executor();
   EXPECT_NO_THROW(manager.reset());
 }
+
+// An isolated manager templated on a non-single-threaded executor and built
+// without an explicit thread count (num_threads_ == 0, "auto") must resolve the
+// `thread_num` parameter when loading a component instead of handing 0 to the
+// executor.
+TEST_F(TestComponentManager, isolated_multi_threaded_auto_thread_num)
+{
+  auto exec = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
+  using IsolatedMultiThreaded =
+    rclcpp_components::ComponentManagerIsolated<rclcpp::executors::MultiThreadedExecutor>;
+  auto manager = std::make_shared<IsolatedMultiThreaded>(exec);
+  auto client_node = rclcpp::Node::make_shared("test_component_manager_isolated_mt");
+
+  exec->add_node(manager);
+  exec->add_node(client_node);
+
+  auto composition_client = client_node->create_client<composition_interfaces::srv::LoadNode>(
+    "/ComponentManager/_container/load_node");
+  ASSERT_TRUE(composition_client->wait_for_service(20s)) << "service not available after waiting";
+
+  auto request = std::make_shared<composition_interfaces::srv::LoadNode::Request>();
+  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);
+  auto result = future.get();
+  EXPECT_TRUE(result->success);
+  EXPECT_EQ(result->error_message, "");
+  EXPECT_EQ(result->full_node_name, "/test_component_foo");
+}