From 5943eec8d708e442e3c56ae490ef7f468895992a Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Sat, 8 Aug 2026 15:44:38 +0900 Subject: [PATCH] address context shutdown racy condition. Signed-off-by: Tomoya Fujita --- rclcpp/src/rclcpp/context.cpp | 24 +++++++++ rclcpp/src/rclcpp/signal_handler.cpp | 17 ++++++- rclcpp/test/rclcpp/test_context.cpp | 73 ++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/context.cpp b/rclcpp/src/rclcpp/context.cpp index 1a7e381b44..3a53e9d66b 100644 --- a/rclcpp/src/rclcpp/context.cpp +++ b/rclcpp/src/rclcpp/context.cpp @@ -29,6 +29,7 @@ #include "rclcpp/exceptions.hpp" #include "rclcpp/logging.hpp" #include "rclcpp/graph_listener.hpp" +#include "rcpputils/scope_exit.hpp" #include "rcutils/error_handling.h" #include "rcutils/macros.h" @@ -316,6 +317,23 @@ Context::shutdown_reason() const return shutdown_reason_; } +/// Contexts that are currently being shutdown by this thread. +/** + * The init_mutex_ is recursive, so it serializes concurrent calls to + * shutdown() from different threads, but it cannot prevent the same thread + * from reentering shutdown(), e.g. when a pre_shutdown callback calls + * shutdown() on the same context again, directly or via rclcpp::shutdown(). + * Such a reentrant call would run the entire shutdown sequence again, + * calling the pre_shutdown callbacks recursively and rcl_shutdown() twice. + * + * This state is intentionally kept out of the Context class so that the + * class layout does not change, keeping this fix ABI compatible. + * A thread_local container is sufficient because concurrent calls from + * other threads are already serialized by init_mutex_; the second thread + * observes is_valid() == false after the first call completes. + */ +static thread_local std::unordered_set g_contexts_in_shutdown; + bool Context::shutdown(const std::string & reason) { @@ -326,6 +344,12 @@ Context::shutdown(const std::string & reason) // if it is not valid, then it cannot be shutdown return false; } + // prevent reentrant calls, e.g. from a pre_shutdown callback + if (!g_contexts_in_shutdown.insert(this).second) { + // shutdown of this context is already in progress on this thread + return false; + } + RCPPUTILS_SCOPE_EXIT(g_contexts_in_shutdown.erase(this); ); // call each pre-shutdown callback { diff --git a/rclcpp/src/rclcpp/signal_handler.cpp b/rclcpp/src/rclcpp/signal_handler.cpp index ebe7585f74..a57470bdbe 100644 --- a/rclcpp/src/rclcpp/signal_handler.cpp +++ b/rclcpp/src/rclcpp/signal_handler.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -266,7 +267,21 @@ SignalHandler::deferred_signal_handler() "deferred_signal_handler(): " "shutting down rclcpp::Context @ %p, because it had shutdown_on_signal == true", static_cast(context_ptr.get())); - context_ptr->shutdown("signal handler"); + try { + context_ptr->shutdown("signal handler"); + } catch (const std::exception & exc) { + // an uncaught exception on this thread would call std::terminate(), + // taking down the whole process, so log the failure instead + RCLCPP_ERROR( + get_logger(), + "deferred_signal_handler(): failed to shutdown rclcpp::Context @ %p: %s", + static_cast(context_ptr.get()), exc.what()); + } catch (...) { + RCLCPP_ERROR( + get_logger(), + "deferred_signal_handler(): failed to shutdown rclcpp::Context @ %p", + static_cast(context_ptr.get())); + } } } } diff --git a/rclcpp/test/rclcpp/test_context.cpp b/rclcpp/test/rclcpp/test_context.cpp index c8779371fe..0374a2d151 100644 --- a/rclcpp/test/rclcpp/test_context.cpp +++ b/rclcpp/test/rclcpp/test_context.cpp @@ -13,6 +13,10 @@ // limitations under the License. #include +#include +#include +#include + #include "rclcpp/context.hpp" #include "rclcpp/rclcpp.hpp" @@ -215,6 +219,75 @@ TEST(TestContext, check_on_shutdown_callback_order_after_del) { EXPECT_TRUE(result[0] == 1 && result[1] == 3 && result[2] == 4 && result[3] == 0); } +// This test checks that a reentrant call to shutdown(), e.g. issued from a +// pre_shutdown callback, returns false instead of running the entire shutdown +// sequence again (which used to recurse into the pre_shutdown callbacks and +// call rcl_shutdown() twice). +TEST(TestContext, reentrant_shutdown_from_pre_shutdown_callback) { + auto context = std::make_shared(); + context->init(0, nullptr); + + size_t pre_shutdown_calls = 0; + size_t on_shutdown_calls = 0; + bool reentrant_shutdown_result = true; + + context->add_pre_shutdown_callback( + [&context, &pre_shutdown_calls, &reentrant_shutdown_result]() { + pre_shutdown_calls++; + reentrant_shutdown_result = context->shutdown("reentrant shutdown"); + }); + context->add_on_shutdown_callback( + [&on_shutdown_calls]() { + on_shutdown_calls++; + }); + + EXPECT_TRUE(context->shutdown("for test")); + EXPECT_FALSE(reentrant_shutdown_result); + EXPECT_EQ(pre_shutdown_calls, 1u); + EXPECT_EQ(on_shutdown_calls, 1u); + EXPECT_FALSE(context->is_valid()); + + // shutdown() must work again after re-initialization + context->init(0, nullptr); + EXPECT_TRUE(context->is_valid()); + EXPECT_TRUE(context->shutdown("for test again")); + EXPECT_FALSE(context->is_valid()); +} + +// This test checks that concurrent calls to shutdown() from multiple threads +// result in exactly one thread running the shutdown sequence, and the other +// threads returning false without error. +TEST(TestContext, concurrent_shutdown) { + auto context = std::make_shared(); + context->init(0, nullptr); + + std::atomic pre_shutdown_calls{0}; + context->add_pre_shutdown_callback( + [&pre_shutdown_calls]() { + pre_shutdown_calls++; + }); + + constexpr size_t num_threads = 8; + std::atomic success_count{0}; + std::vector threads; + threads.reserve(num_threads); + for (size_t i = 0; i < num_threads; ++i) { + threads.emplace_back( + [&context, &success_count]() { + if (context->shutdown("concurrent shutdown")) { + success_count++; + } + }); + } + for (auto & thread : threads) { + thread.join(); + } + + EXPECT_EQ(success_count, 1u); + EXPECT_EQ(pre_shutdown_calls, 1u); + EXPECT_FALSE(context->is_valid()); +} + // This test checks that contexts will be properly destroyed when leaving a scope, after a // guard condition has been created. TEST(TestContext, check_context_destroyed) {