From 41a99b64d347ce2ff8d5fd4ed8ebaa5cfbc5117f Mon Sep 17 00:00:00 2001
From: Jacob Perron <jacob@openrobotics.org>
Date: Wed, 7 Aug 2019 09:26:19 -0700
Subject: [PATCH] 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 <jacob@openrobotics.org>
---
 rclcpp_action/include/rclcpp_action/client.hpp         | 10 +++++-----
 .../include/rclcpp_action/client_goal_handle.hpp       |  3 ++-
 .../include/rclcpp_action/client_goal_handle_impl.hpp  |  4 +++-
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/rclcpp_action/include/rclcpp_action/client.hpp b/rclcpp_action/include/rclcpp_action/client.hpp
index e2f7bad..d94508f 100644
--- a/rclcpp_action/include/rclcpp_action/client.hpp
+++ b/rclcpp_action/include/rclcpp_action/client.hpp
@@ -410,10 +410,7 @@ public:
       // 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 @@ private:
   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 @@ private:
         std::lock_guard<std::mutex> lock(goal_handles_mutex_);
         goal_handles_.erase(goal_handle->get_goal_id());
       });
-    goal_handle->set_result_awareness(true);
   }
 
   /// \internal
diff --git a/rclcpp_action/include/rclcpp_action/client_goal_handle.hpp b/rclcpp_action/include/rclcpp_action/client_goal_handle.hpp
index 28210fa..42e2844 100644
--- a/rclcpp_action/include/rclcpp_action/client_goal_handle.hpp
+++ b/rclcpp_action/include/rclcpp_action/client_goal_handle.hpp
@@ -134,7 +134,8 @@ private:
     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
diff --git a/rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp b/rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp
index c588e71..402bd25 100644
--- a/rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp
+++ b/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>