From eedb3e6b5327f0dca681f95d9984d1dcca8904c9 Mon Sep 17 00:00:00 2001 From: Stephen Brawner Date: Tue, 8 Sep 2020 16:11:54 -0700 Subject: [PATCH 1/2] Set promise value after try-catch Signed-off-by: Stephen Brawner --- rclcpp_action/include/rclcpp_action/client.hpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/rclcpp_action/include/rclcpp_action/client.hpp b/rclcpp_action/include/rclcpp_action/client.hpp index a46e008f33..090790a513 100644 --- a/rclcpp_action/include/rclcpp_action/client.hpp +++ b/rclcpp_action/include/rclcpp_action/client.hpp @@ -370,14 +370,6 @@ class Client : public ClientBase // Do not use std::make_shared as friendship cannot be forwarded. std::shared_ptr goal_handle( new GoalHandle(goal_info, options.feedback_callback, options.result_callback)); - { - std::lock_guard guard(goal_handles_mutex_); - goal_handles_[goal_handle->get_goal_id()] = goal_handle; - } - promise->set_value(goal_handle); - if (options.goal_response_callback) { - options.goal_response_callback(future); - } if (options.result_callback) { try { @@ -387,6 +379,16 @@ class Client : public ClientBase return; } } + + { + std::lock_guard guard(goal_handles_mutex_); + goal_handles_[goal_handle->get_goal_id()] = goal_handle; + } + + promise->set_value(goal_handle); + if (options.goal_response_callback) { + options.goal_response_callback(future); + } }); // TODO(jacobperron): Encapsulate into it's own function and From 84f383feb0cc9a14daaa1f67c8e91cb52c9743fc Mon Sep 17 00:00:00 2001 From: Stephen Brawner Date: Thu, 10 Sep 2020 15:29:09 -0700 Subject: [PATCH 2/2] Move goal_response_callback earlier Signed-off-by: Stephen Brawner --- rclcpp_action/include/rclcpp_action/client.hpp | 11 ++++++----- rclcpp_action/test/test_client.cpp | 3 +-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/rclcpp_action/include/rclcpp_action/client.hpp b/rclcpp_action/include/rclcpp_action/client.hpp index 090790a513..869875f82a 100644 --- a/rclcpp_action/include/rclcpp_action/client.hpp +++ b/rclcpp_action/include/rclcpp_action/client.hpp @@ -262,7 +262,7 @@ class Client : public ClientBase using GoalHandle = ClientGoalHandle; using WrappedResult = typename GoalHandle::WrappedResult; using GoalResponseCallback = - std::function)>; + std::function; using FeedbackCallback = typename GoalHandle::FeedbackCallback; using ResultCallback = typename GoalHandle::ResultCallback; using CancelRequest = typename ActionT::Impl::CancelGoalService::Request; @@ -360,7 +360,7 @@ class Client : public ClientBase if (!goal_response->accepted) { promise->set_value(nullptr); if (options.goal_response_callback) { - options.goal_response_callback(future); + options.goal_response_callback(nullptr); } return; } @@ -371,6 +371,10 @@ class Client : public ClientBase std::shared_ptr goal_handle( new GoalHandle(goal_info, options.feedback_callback, options.result_callback)); + if (options.goal_response_callback) { + options.goal_response_callback(goal_handle); + } + if (options.result_callback) { try { this->make_result_aware(goal_handle); @@ -386,9 +390,6 @@ class Client : public ClientBase } promise->set_value(goal_handle); - if (options.goal_response_callback) { - options.goal_response_callback(future); - } }); // TODO(jacobperron): Encapsulate into it's own function and diff --git a/rclcpp_action/test/test_client.cpp b/rclcpp_action/test/test_client.cpp index 84c9dbe7d0..13cd07add4 100644 --- a/rclcpp_action/test/test_client.cpp +++ b/rclcpp_action/test/test_client.cpp @@ -435,9 +435,8 @@ TEST_F(TestClientAgainstServer, async_send_goal_with_goal_response_callback_wait auto send_goal_ops = rclcpp_action::Client::SendGoalOptions(); send_goal_ops.goal_response_callback = [&goal_response_received] - (std::shared_future future) mutable + (typename ActionGoalHandle::SharedPtr goal_handle) mutable { - auto goal_handle = future.get(); if (goal_handle) { goal_response_received = true; }