From 07081f597a53bb5569fcc1aa9f59c2424cdf52da Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Thu, 13 Aug 2026 01:35:07 +0900 Subject: [PATCH] address context shutdown racy condition. (#3219) Signed-off-by: Tomoya Fujita (cherry picked from commit b723878c1a1d186a1b94883ea4adbe7093209da7) # Conflicts: # rclcpp/src/rclcpp/context.cpp # rclcpp/test/rclcpp/test_context.cpp --- rclcpp/src/rclcpp/context.cpp | 26 +++++++++++++++++++++++++- rclcpp/src/rclcpp/signal_handler.cpp | 17 ++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/rclcpp/src/rclcpp/context.cpp b/rclcpp/src/rclcpp/context.cpp index 33bd0bf0b9..dd0b81f950 100644 --- a/rclcpp/src/rclcpp/context.cpp +++ b/rclcpp/src/rclcpp/context.cpp @@ -28,7 +28,8 @@ #include "rclcpp/detail/utilities.hpp" #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" @@ -296,6 +297,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) { @@ -306,6 +324,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 c0c6b70fea..4c5c3e08bd 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 @@ -264,7 +265,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())); + } } } }