Guard against making multiple result requests for a goal handle (#808)

This fixes a runtime error caused by a race condition when making consecutive requests for the
result.
Specifically, this happens if the user provides a result callback when sending a goal and then
calls async_get_result shortly after.

Resolves #783

Signed-off-by: Jacob Perron <[email protected]>

Author: jacobperron

rclcpp_action/include/rclcpp_action/client.hpp

@@ -410,10 +410,7 @@ class Client : public ClientBase
       // This will override any previously registered callback
       goal_handle->set_result_callback(result_callback);
     }
-    // If the user chose to ignore the result before, then ask the server for the result now.
-    if (!goal_handle->is_result_aware()) {
-      this->make_result_aware(goal_handle);
-    }
+    this->make_result_aware(goal_handle);
     return goal_handle->async_result();
   }
 
@@ -595,6 +592,10 @@ class Client : public ClientBase
   void
   make_result_aware(typename GoalHandle::SharedPtr goal_handle)
   {
+    // Avoid making more than one request
+    if (goal_handle->set_result_awareness(true)) {
+      return;
+    }
     using GoalResultRequest = typename ActionT::Impl::GetResultService::Request;
     auto goal_result_request = std::make_shared<GoalResultRequest>();
     goal_result_request->goal_id.uuid = goal_handle->get_goal_id();
@@ -614,7 +615,6 @@ class Client : public ClientBase
         std::lock_guard<std::mutex> lock(goal_handles_mutex_);
         goal_handles_.erase(goal_handle->get_goal_id());
       });
-    goal_handle->set_result_awareness(true);
   }
 
   /// \internal

rclcpp_action/include/rclcpp_action/client_goal_handle.hpp

@@ -134,7 +134,8 @@ class ClientGoalHandle
     typename ClientGoalHandle<ActionT>::SharedPtr shared_this,
     typename std::shared_ptr<const Feedback> feedback_message);
 
-  void
+  /// Returns the previous value of awareness
+  bool
   set_result_awareness(bool awareness);
 
   void

rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp

@@ -127,11 +127,13 @@ ClientGoalHandle<ActionT>::is_result_aware()
 }
 
 template<typename ActionT>
-void
+bool
 ClientGoalHandle<ActionT>::set_result_awareness(bool awareness)
 {
   std::lock_guard<std::mutex> guard(handle_mutex_);
+  bool previous = is_result_aware_;
   is_result_aware_ = awareness;
+  return previous;
 }
 
 template<typename ActionT>