fix: Don't deadlock if removing shutdown callbacks in a shutdown callback (#2886)

Signed-off-by: Janosch Machowinski <[email protected]>
Co-authored-by: Janosch Machowinski <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>

Author: 3 people

rclcpp/include/rclcpp/context.hpp

@@ -381,10 +381,10 @@ class Context : public std::enable_shared_from_this<Context>
   std::recursive_mutex sub_contexts_mutex_;
 
   std::vector<std::shared_ptr<OnShutdownCallback>> on_shutdown_callbacks_;
-  mutable std::mutex on_shutdown_callbacks_mutex_;
+  mutable std::recursive_mutex on_shutdown_callbacks_mutex_;
 
   std::vector<std::shared_ptr<PreShutdownCallback>> pre_shutdown_callbacks_;
-  mutable std::mutex pre_shutdown_callbacks_mutex_;
+  mutable std::recursive_mutex pre_shutdown_callbacks_mutex_;
 
   /// Condition variable for timed sleep (see sleep_for).
   std::condition_variable interrupt_condition_variable_;

rclcpp/src/rclcpp/context.cpp

@@ -310,9 +310,16 @@ Context::shutdown(const std::string & reason)
 
   // call each pre-shutdown callback
   {
-    std::lock_guard<std::mutex> lock{pre_shutdown_callbacks_mutex_};
-    for (const auto & callback : pre_shutdown_callbacks_) {
-      (*callback)();
+    std::lock_guard<std::recursive_mutex> lock{pre_shutdown_callbacks_mutex_};
+    // callbacks may delete other callbacks during the execution,
+    // therefore we need to save a copy and check before execution
+    // if the next callback is still present
+    auto cpy = pre_shutdown_callbacks_;
+    for (const auto & callback : cpy) {
+      auto it = std::find(pre_shutdown_callbacks_.begin(), pre_shutdown_callbacks_.end(), callback);
+      if(it != pre_shutdown_callbacks_.end()) {
+        (*callback)();
+      }
     }
   }
 
@@ -325,9 +332,16 @@ Context::shutdown(const std::string & reason)
   shutdown_reason_ = reason;
   // call each shutdown callback
   {
-    std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
-    for (const auto & callback : on_shutdown_callbacks_) {
-      (*callback)();
+    std::lock_guard<std::recursive_mutex> lock(on_shutdown_callbacks_mutex_);
+    // callbacks may delete other callbacks during the execution,
+    // therefore we need to save a copy and check before execution
+    // if the next callback is still present
+    auto cpy = on_shutdown_callbacks_;
+    for (const auto & callback : cpy) {
+      auto it = std::find(on_shutdown_callbacks_.begin(), on_shutdown_callbacks_.end(), callback);
+      if(it != on_shutdown_callbacks_.end()) {
+        (*callback)();
+      }
     }
   }
 
@@ -398,10 +412,10 @@ Context::add_shutdown_callback(
     shutdown_type == ShutdownType::pre_shutdown || shutdown_type == ShutdownType::on_shutdown);
 
   if constexpr (shutdown_type == ShutdownType::pre_shutdown) {
-    std::lock_guard<std::mutex> lock(pre_shutdown_callbacks_mutex_);
+    std::lock_guard<std::recursive_mutex> lock(pre_shutdown_callbacks_mutex_);
     pre_shutdown_callbacks_.emplace_back(callback_shared_ptr);
   } else {
-    std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
+    std::lock_guard<std::recursive_mutex> lock(on_shutdown_callbacks_mutex_);
     on_shutdown_callbacks_.emplace_back(callback_shared_ptr);
   }
 
@@ -421,7 +435,7 @@ Context::remove_shutdown_callback(
   }
 
   const auto remove_callback = [&callback_shared_ptr](auto & mutex, auto & callback_vector) {
-      const std::lock_guard<std::mutex> lock(mutex);
+      const std::lock_guard<std::recursive_mutex> lock(mutex);
       auto iter = callback_vector.begin();
       for (; iter != callback_vector.end(); iter++) {
         if ((*iter).get() == callback_shared_ptr.get()) {
@@ -462,7 +476,7 @@ std::vector<rclcpp::Context::ShutdownCallback>
 Context::get_shutdown_callback() const
 {
   const auto get_callback_vector = [](auto & mutex, auto & callback_set) {
-      const std::lock_guard<std::mutex> lock(mutex);
+      const std::lock_guard<std::recursive_mutex> lock(mutex);
       std::vector<rclcpp::Context::ShutdownCallback> callbacks;
       for (auto & callback : callback_set) {
         callbacks.push_back(*callback);