Fix occasionally missing goal result caused by race condition #1677

KavenYau · 2021-05-17T13:30:14Z

Fix occasionally missing goal result when a action finished quickly (one thread enter to execute_result_request_received and the other enter to publish_result).

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

fujitatomoya

this makes sense to me.

ivanpauno · 2021-05-18T13:11:27Z

rclcpp_action/src/server.cpp

@@ -627,19 +627,19 @@ ServerBase::publish_result(const GoalUUID & uuid, std::shared_ptr<void> result_m
  }

  {
-    std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_);
+    std::lock_guard<std::recursive_mutex> unordered_map_lock(pimpl_->unordered_map_mutex_);


This seems fine to me, but it can introduce a ptential deadlock in the future.
I would rather:

Document in which order we're currently taking the unordered_map_mutex_, action_server_reentrant_mutex_ mutexes, to avoid someone doing it in the opposite order later.

Take action_server_reentrant_mutex_ out of the if/loop together with unordered_map_mutex_ using std::scoped_lock.

With using std::scoped_lock., then it could not be backported to foxy? Because foxy is compiled with C++14.

Sounds good, but please add documentation explaining what's the current order we're taking the mutexes.

I added a NOTE comment to this block scope.

rclcpp_action/src/server.cpp

ivanpauno · 2021-05-18T13:14:47Z

rclcpp_action/src/server.cpp

@@ -500,9 +500,13 @@ ServerBase::execute_result_request_received(std::shared_ptr<void> & data)
    result_response = create_result_response(action_msgs::msg::GoalStatus::STATUS_UNKNOWN);
  } else {
    // Goal exists, check if a result is already available
+    std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_);


nit:

Suggested change

std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_);

std::lock_guard lock(pimpl_->unordered_map_mutex_);

ivanpauno · 2021-05-18T13:15:08Z

rclcpp_action/src/server.cpp

@@ -627,19 +627,19 @@ ServerBase::publish_result(const GoalUUID & uuid, std::shared_ptr<void> result_m
  }

  {
-    std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_);
+    std::lock_guard<std::recursive_mutex> unordered_map_lock(pimpl_->unordered_map_mutex_);


nit:

Suggested change

std::lock_guard<std::recursive_mutex> unordered_map_lock(pimpl_->unordered_map_mutex_);

std::lock_guard unordered_map_lock(pimpl_->unordered_map_mutex_);

This two changes are inconsistent with other std::lock_guard in server.cpp.

Before, we were using c++14 and this was not possible, so we're going to find inconsistencies.
The idea is to use the less verbose version in new code.

In this case, a backport PR will need to be modified to be compliant with c++14 again.
I think that's fine, but it's also fine if you want to keep the verbose version here.

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

fujitatomoya · 2021-05-20T04:57:51Z

CI:

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2021-05-20T20:15:55Z

Failures are unrelated

fujitatomoya · 2021-05-20T23:29:30Z

@ivanpauno we would want this to backport to foxy and galactic? what you think?

SteveMacenski · 2021-05-21T00:50:29Z

I would like that 😄

) * 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>

fujitatomoya · 2021-05-21T06:42:08Z

backpork PRs are open for foxy and galactic.

[foxy] Fix occasionally missing goal result caused by race condition (#1677) #1682
[Galactic] Fix occasionally missing goal result caused by race condition (#1677) #1683

…#1683) * 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>

…#1682) * 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>

Fix occasionally missing goal result caused by race condition

32843d7

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

fujitatomoya approved these changes May 18, 2021

View reviewed changes

fujitatomoya requested review from ivanpauno and sloretz May 18, 2021 12:08

fujitatomoya added the bug Something isn't working label May 18, 2021

KavenYau mentioned this pull request May 18, 2021

BT Navigator got stuck because goal_status of bt_action_node was always STATUS_SUCCEEDED open-navigation/navigation2#2340

Closed

ivanpauno reviewed May 18, 2021

View reviewed changes

KavenYau added 2 commits May 20, 2021 09:00

Take action_server_reentrant_mutex_ out of the sending result loop

fef7b01

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

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

2492d7c

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

KavenYau force-pushed the fix-occasionally-missing-goal-result branch from 6b7ef43 to 2492d7c Compare May 20, 2021 01:17

ivanpauno approved these changes May 20, 2021

View reviewed changes

ivanpauno merged commit 1c61751 into ros2:master May 20, 2021

KavenYau deleted the fix-occasionally-missing-goal-result branch May 21, 2021 01:11

jacobperron mentioned this pull request May 25, 2021

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

Merged

BriceRenaudeau mentioned this pull request Aug 27, 2021

ActionClient result callback not triggered if the action is too quick #1757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix occasionally missing goal result caused by race condition #1677

Fix occasionally missing goal result caused by race condition #1677

KavenYau commented May 17, 2021

fujitatomoya left a comment

ivanpauno May 18, 2021

KavenYau May 19, 2021 •

edited

ivanpauno May 19, 2021

KavenYau May 20, 2021

ivanpauno May 18, 2021

ivanpauno May 18, 2021

KavenYau May 19, 2021

ivanpauno May 19, 2021

fujitatomoya commented May 20, 2021

ivanpauno commented May 20, 2021

fujitatomoya commented May 20, 2021

SteveMacenski commented May 21, 2021

fujitatomoya commented May 21, 2021

	std::lock_guard<std::recursive_mutex> lock(pimpl_->unordered_map_mutex_);
	std::lock_guard lock(pimpl_->unordered_map_mutex_);

	std::lock_guard<std::recursive_mutex> unordered_map_lock(pimpl_->unordered_map_mutex_);
	std::lock_guard unordered_map_lock(pimpl_->unordered_map_mutex_);

Fix occasionally missing goal result caused by race condition #1677

Fix occasionally missing goal result caused by race condition #1677

Conversation

KavenYau commented May 17, 2021

fujitatomoya left a comment

Choose a reason for hiding this comment

ivanpauno May 18, 2021

Choose a reason for hiding this comment

KavenYau May 19, 2021 • edited

Choose a reason for hiding this comment

ivanpauno May 19, 2021

Choose a reason for hiding this comment

KavenYau May 20, 2021

Choose a reason for hiding this comment

ivanpauno May 18, 2021

Choose a reason for hiding this comment

ivanpauno May 18, 2021

Choose a reason for hiding this comment

KavenYau May 19, 2021

Choose a reason for hiding this comment

ivanpauno May 19, 2021

Choose a reason for hiding this comment

fujitatomoya commented May 20, 2021

ivanpauno commented May 20, 2021

fujitatomoya commented May 20, 2021

SteveMacenski commented May 21, 2021

fujitatomoya commented May 21, 2021

KavenYau May 19, 2021 •

edited