fix(ClientGoalHandle): Made mutex recursive to prevent deadlocks

This prevents deadlocks in cases, were e.g. get_status() would
be called on the handle in a callback of the handle.

Signed-off-by: Janosch Machowinski <[email protected]>

Author: Janosch Machowinski

rclcpp_action/include/rclcpp_action/client_goal_handle.hpp

@@ -163,7 +163,7 @@ class ClientGoalHandle
   ResultCallback result_callback_{nullptr};
   int8_t status_{GoalStatus::STATUS_ACCEPTED};
 
-  std::mutex handle_mutex_;
+  std::recursive_mutex handle_mutex_;
 };
 }  // namespace rclcpp_action
 

rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp

@@ -59,7 +59,7 @@ template<typename ActionT>
 std::shared_future<typename ClientGoalHandle<ActionT>::WrappedResult>
 ClientGoalHandle<ActionT>::async_get_result()
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   if (!is_result_aware_) {
     throw exceptions::UnawareGoalHandleError();
   }
@@ -70,7 +70,7 @@ template<typename ActionT>
 void
 ClientGoalHandle<ActionT>::set_result(const WrappedResult & wrapped_result)
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   status_ = static_cast<int8_t>(wrapped_result.code);
   result_promise_.set_value(wrapped_result);
   if (result_callback_) {
@@ -82,55 +82,55 @@ template<typename ActionT>
 void
 ClientGoalHandle<ActionT>::set_feedback_callback(FeedbackCallback callback)
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   feedback_callback_ = callback;
 }
 
 template<typename ActionT>
 void
 ClientGoalHandle<ActionT>::set_result_callback(ResultCallback callback)
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   result_callback_ = callback;
 }
 
 template<typename ActionT>
 int8_t
 ClientGoalHandle<ActionT>::get_status()
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   return status_;
 }
 
 template<typename ActionT>
 void
 ClientGoalHandle<ActionT>::set_status(int8_t status)
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   status_ = status;
 }
 
 template<typename ActionT>
 bool
 ClientGoalHandle<ActionT>::is_feedback_aware()
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   return feedback_callback_ != nullptr;
 }
 
 template<typename ActionT>
 bool
 ClientGoalHandle<ActionT>::is_result_aware()
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   return is_result_aware_;
 }
 
 template<typename ActionT>
 bool
 ClientGoalHandle<ActionT>::set_result_awareness(bool awareness)
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   bool previous = is_result_aware_;
   is_result_aware_ = awareness;
   return previous;
@@ -140,7 +140,7 @@ template<typename ActionT>
 void
 ClientGoalHandle<ActionT>::invalidate(const exceptions::UnawareGoalHandleError & ex)
 {
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   // Guard against multiple calls
   if (is_invalidated()) {
     return;
@@ -168,7 +168,7 @@ ClientGoalHandle<ActionT>::call_feedback_callback(
     RCLCPP_ERROR(rclcpp::get_logger("rclcpp_action"), "Sent feedback to wrong goal handle.");
     return;
   }
-  std::lock_guard<std::mutex> guard(handle_mutex_);
+  std::lock_guard<std::recursive_mutex> guard(handle_mutex_);
   if (nullptr == feedback_callback_) {
     // Normal, some feedback messages may arrive after the goal result.
     RCLCPP_DEBUG(rclcpp::get_logger("rclcpp_action"), "Received feedback but goal ignores it.");