From d1fe859bd6ceca0d01d159513ccdb4d1c6285b7f Mon Sep 17 00:00:00 2001 From: Kaven Yau Date: Mon, 26 Apr 2021 20:40:09 +0800 Subject: [PATCH 1/3] modify unit test for fixing #1599 Signed-off-by: Kaven Yau --- rclcpp_action/test/test_server.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/rclcpp_action/test/test_server.cpp b/rclcpp_action/test/test_server.cpp index af94d67cbf..1a0b074ba3 100644 --- a/rclcpp_action/test/test_server.cpp +++ b/rclcpp_action/test/test_server.cpp @@ -1234,14 +1234,10 @@ class TestDeadlockServer : public TestServer this->TryLockFor(lock, std::chrono::milliseconds(1000)); return rclcpp_action::GoalResponse::ACCEPT_AND_EXECUTE; }, - [this](std::shared_ptr handle) { + [this](std::shared_ptr) { // instead of making a deadlock, check if it can acquire the lock in a second std::unique_lock lock(server_mutex_, std::defer_lock); this->TryLockFor(lock, std::chrono::milliseconds(1000)); - // TODO(KavenYau): this check may become obsolete with https://github.com/ros2/rclcpp/issues/1599 - if (!handle->is_active()) { - return rclcpp_action::CancelResponse::REJECT; - } return rclcpp_action::CancelResponse::ACCEPT; }, [this](std::shared_ptr handle) { @@ -1316,6 +1312,9 @@ TEST_F(TestDeadlockServer, deadlock_while_succeed_and_canceled) send_goal_request(node_, uuid1_); std::thread t(&TestDeadlockServer::GoalSucceeded, this); rclcpp::sleep_for(std::chrono::milliseconds(50)); - send_cancel_request(node_, uuid1_); + auto response_ptr = send_cancel_request(node_, uuid1_); + + // current goal handle is not cancelable, so it returns ERROR_REJECTED + EXPECT_EQ(CancelResponse::ERROR_REJECTED, response_ptr->return_code); t.join(); } From 8cac085162339348a90747bad4de814453992645 Mon Sep 17 00:00:00 2001 From: Kaven Yau Date: Tue, 27 Apr 2021 14:52:47 +0800 Subject: [PATCH 2/3] return CancelResponse::REJECT while failed to transit to CANCELING state Signed-off-by: Kaven Yau --- rclcpp_action/include/rclcpp_action/server.hpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rclcpp_action/include/rclcpp_action/server.hpp b/rclcpp_action/include/rclcpp_action/server.hpp index adc38c7b96..f93660b315 100644 --- a/rclcpp_action/include/rclcpp_action/server.hpp +++ b/rclcpp_action/include/rclcpp_action/server.hpp @@ -369,7 +369,11 @@ class Server : public ServerBase, public std::enable_shared_from_this_cancel_goal(); + try { + goal_handle->_cancel_goal(); + } catch (const rclcpp::exceptions::RCLError & ex) { + return CancelResponse::REJECT; + } } } return resp; From ecca6f7e787588056a7c07f838bc0d0fa1f4b4a8 Mon Sep 17 00:00:00 2001 From: Kaven Yau Date: Wed, 28 Apr 2021 09:01:47 +0800 Subject: [PATCH 3/3] add log message for cancel goal failed Signed-off-by: Kaven Yau --- rclcpp_action/include/rclcpp_action/server.hpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rclcpp_action/include/rclcpp_action/server.hpp b/rclcpp_action/include/rclcpp_action/server.hpp index f93660b315..f9488fc1d5 100644 --- a/rclcpp_action/include/rclcpp_action/server.hpp +++ b/rclcpp_action/include/rclcpp_action/server.hpp @@ -372,6 +372,9 @@ class Server : public ServerBase, public std::enable_shared_from_this_cancel_goal(); } catch (const rclcpp::exceptions::RCLError & ex) { + RCLCPP_DEBUG( + rclcpp::get_logger("rclcpp_action"), + "Failed to cancel goal in call_handle_cancel_callback: %s", ex.what()); return CancelResponse::REJECT; } }