address context shutdown racy condition. (backport #3219)

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

rclcpp/src/rclcpp/signal_handler.cpp

@@ -16,6 +16,7 @@
 
 #include <atomic>
 #include <csignal>
+#include <exception>
 #include <mutex>
 #include <string>
 #include <thread>
@@ -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<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()));
+          }
         }
       }
     }