niklas/rclcpp
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix occasionally missing goal result caused by race condition (#1677) (#1682)

Browse source 
* Fix occasionally missing goal result caused by race condition

Signed-off-by: Kaven Yau <kavenyau@foxmail.com>

* Take action_server_reentrant_mutex_ out of the sending result loop

Signed-off-by: Kaven Yau <kavenyau@foxmail.com>

* add note for explaining the current locking order in server.cpp

Signed-off-by: Kaven Yau <kavenyau@foxmail.com>

Co-authored-by: Kaven Yau <kavenyau@foxmail.com>
This commit is contained in:
Tomoya Fujita 2021-05-26 02:28:02 +09:00 • committed by GitHub
parent 122355704b
commit 1c92622516
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
1 changed files with 25 additions and 14 deletions
Download patch file Download diff file Expand all files Collapse all files

25
rclcpp_action/src/server.cpp
View file

@ -463,9 +463,13 @@ ServerBase::execute_result_request_received()
result_response = create_result_response(action_msgs::msg::GoalStatus::STATUS_UNKNOWN); result_response = create_result_response(action_msgs::msg::GoalStatus::STATUS_UNKNOWN);
} else { } else {
// Goal exists, check if a result is already available // Goal exists, check if a result is already available
std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_);
auto iter = pimpl_->goal_results_.find(uuid); auto iter = pimpl_->goal_results_.find(uuid);
if (iter != pimpl_->goal_results_.end()) { if (iter != pimpl_->goal_results_.end()) {
result_response = iter->second; result_response = iter->second;
} else {
// Store the request so it can be responded to later
pimpl_->result_requests_[uuid].push_back(request_header);
} }
} }
@ -477,10 +481,6 @@ ServerBase::execute_result_request_received()
if (RCL_RET_OK != ret) { if (RCL_RET_OK != ret) {
rclcpp::exceptions::throw_from_rcl_error(ret); rclcpp::exceptions::throw_from_rcl_error(ret);
} }
} else {
// Store the request so it can be responded to later
std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_);
pimpl_->result_requests_[uuid].push_back(request_header);
} }
} }
@ -589,15 +589,25 @@ ServerBase::publish_result(const GoalUUID & uuid, std::shared_ptr<void> result_m
} }
{ {
std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_); /**
* NOTE: There is a potential deadlock issue if both unordered_map_mutex_ and
* action_server_reentrant_mutex_ locked in other block scopes. Unless using
* std::scoped_lock, locking order must be consistent with the current.
*
* Current locking order:
*
* 1. unordered_map_mutex_
* 2. action_server_reentrant_mutex_
*
*/
std::lock_guard<std::recursive_mutex> unordered_map_lock(pimpl_->unordered_map_mutex_);
pimpl_->goal_results_[uuid] = result_msg; pimpl_->goal_results_[uuid] = result_msg;
}
// if there are clients who already asked for the result, send it to them // if there are clients who already asked for the result, send it to them
auto iter = pimpl_->result_requests_.find(uuid); auto iter = pimpl_->result_requests_.find(uuid);
if (iter != pimpl_->result_requests_.end()) { if (iter != pimpl_->result_requests_.end()) {
for (auto & request_header : iter->second) {
std::lock_guard<std::recursive_mutex> lock(pimpl_->action_server_reentrant_mutex_); std::lock_guard<std::recursive_mutex> lock(pimpl_->action_server_reentrant_mutex_);
for (auto & request_header : iter->second) {
rcl_ret_t ret = rcl_action_send_result_response( rcl_ret_t ret = rcl_action_send_result_response(
pimpl_->action_server_.get(), &request_header, result_msg.get()); pimpl_->action_server_.get(), &request_header, result_msg.get());
if (RCL_RET_OK != ret) { if (RCL_RET_OK != ret) {
@ -606,6 +616,7 @@ ServerBase::publish_result(const GoalUUID & uuid, std::shared_ptr<void> result_m
} }
} }
} }
}
void void
ServerBase::notify_goal_terminal_state() ServerBase::notify_goal_terminal_state()