address context shutdown racy condition.

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<const Context *> 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
   {

rclcpp/src/rclcpp/signal_handler.cpp

@@ -16,6 +16,7 @@
 
 #include <atomic>
 #include <csignal>
+#include <exception>
 #include <mutex>
 #include <string>
 #include <thread>
@@ -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<void *>(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<void *>(context_ptr.get()), exc.what());
+          } catch (...) {
+            RCLCPP_ERROR(
+              get_logger(),
+              "deferred_signal_handler(): failed to shutdown rclcpp::Context @ %p",
+              static_cast<void *>(context_ptr.get()));
+          }
         }
       }
     }

rclcpp/test/rclcpp/test_context.cpp

@@ -13,6 +13,10 @@
 // limitations under the License.
 #include <gtest/gtest.h>
 
+#include <atomic>
+#include <thread>
+#include <vector>
+
 #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<rclcpp::Context>();
+  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<rclcpp::Context>();
+  context->init(0, nullptr);
+
+  std::atomic<size_t> pre_shutdown_calls{0};
+  context->add_pre_shutdown_callback(
+    [&pre_shutdown_calls]() {
+      pre_shutdown_calls++;
+    });
+
+  constexpr size_t num_threads = 8;
+  std::atomic<size_t> success_count{0};
+  std::vector<std::thread> 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) {