From 38fb2cf42c88dd355bd8f3d1ef19d79b8a872279 Mon Sep 17 00:00:00 2001 From: atsushi421 Date: Thu, 23 Apr 2026 06:25:39 +0900 Subject: [PATCH 1/2] fix(cie): update checkout action and fix cancel_executor race - Update actions/checkout from v2 to v4 in run-pre-commit workflow for consistency with build-and-test workflow and security. - Fix race condition in cancel_executor where thread_initialized could be true before spin() sets spinning=true, causing cancel() to have no effect. Always wait for is_spinning() before cancelling. Signed-off-by: atsushi421 --- .github/workflows/run-pre-commit.yaml | 2 +- .../src/component_container_callback_isolated.cpp | 13 ++++--------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/.github/workflows/run-pre-commit.yaml b/.github/workflows/run-pre-commit.yaml index 05d61ba..7ee953b 100644 --- a/.github/workflows/run-pre-commit.yaml +++ b/.github/workflows/run-pre-commit.yaml @@ -10,7 +10,7 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Set up Python uses: actions/setup-python@v4 diff --git a/callback_isolated_executor/src/component_container_callback_isolated.cpp b/callback_isolated_executor/src/component_container_callback_isolated.cpp index 02dc9e6..a191fe9 100644 --- a/callback_isolated_executor/src/component_container_callback_isolated.cpp +++ b/callback_isolated_executor/src/component_container_callback_isolated.cpp @@ -17,14 +17,13 @@ class ComponentManagerCallbackIsolated struct ExecutorWrapper { explicit ExecutorWrapper(std::shared_ptr executor) - : executor(executor), thread_initialized(false) {} + : executor(executor) {} ExecutorWrapper(const ExecutorWrapper &) = delete; ExecutorWrapper &operator=(const ExecutorWrapper &) = delete; std::shared_ptr executor; std::thread thread; - std::atomic_bool thread_initialized; }; public: @@ -176,7 +175,6 @@ void ComponentManagerCallbackIsolated::add_node_to_executor(uint64_t node_id) { } } - executor_wrapper.thread_initialized = true; executor_wrapper.executor->spin(); }); } else { @@ -199,7 +197,6 @@ void ComponentManagerCallbackIsolated::add_node_to_executor(uint64_t node_id) { this->client_publisher_, tid, group_id); } - executor_wrapper.thread_initialized = true; executor_wrapper.executor->spin(); }); } @@ -221,12 +218,10 @@ void ComponentManagerCallbackIsolated::remove_node_from_executor( void ComponentManagerCallbackIsolated::cancel_executor( ExecutorWrapper &executor_wrapper) { - if (!executor_wrapper.thread_initialized) { - auto context = this->get_node_base_interface()->get_context(); + auto context = this->get_node_base_interface()->get_context(); - while (!executor_wrapper.executor->is_spinning() && rclcpp::ok(context)) { - rclcpp::sleep_for(std::chrono::milliseconds(1)); - } + while (!executor_wrapper.executor->is_spinning() && rclcpp::ok(context)) { + rclcpp::sleep_for(std::chrono::milliseconds(1)); } executor_wrapper.executor->cancel(); From 9271f2c730f8c8462f18d0296df233cfeccc8d7f Mon Sep 17 00:00:00 2001 From: atsushi421 Date: Thu, 23 Apr 2026 06:29:55 +0900 Subject: [PATCH 2/2] fix(cie): address review findings - Add comment explaining why cancel_executor must always wait for is_spinning() to prevent re-introduction of the race condition. - Add joinable() guard to avoid undefined behavior when the thread was never started. Signed-off-by: atsushi421 --- .../src/component_container_callback_isolated.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/callback_isolated_executor/src/component_container_callback_isolated.cpp b/callback_isolated_executor/src/component_container_callback_isolated.cpp index a191fe9..6148ed8 100644 --- a/callback_isolated_executor/src/component_container_callback_isolated.cpp +++ b/callback_isolated_executor/src/component_container_callback_isolated.cpp @@ -218,6 +218,14 @@ void ComponentManagerCallbackIsolated::remove_node_from_executor( void ComponentManagerCallbackIsolated::cancel_executor( ExecutorWrapper &executor_wrapper) { + if (!executor_wrapper.thread.joinable()) { + return; + } + + // Always wait for is_spinning() before cancel(). Do not use a separate + // "thread_initialized" flag as a fast-path: such a flag is set before spin() + // sets spinning=true, creating a window where cancel() has no effect and the + // subsequent spin() runs indefinitely, blocking join() forever. auto context = this->get_node_base_interface()->get_context(); while (!executor_wrapper.executor->is_spinning() && rclcpp::ok(context)) {