Fix action server deadlock issue that caused by other mutexes locked in CancelCallback #1635

KavenYau · 2021-04-15T23:58:09Z

Resolves the deadlock issue mentioned in ros-navigation/navigation2#2273 and ros-navigation/navigation2#2304

The unit test which I added could also reproduce the issue.

Signed-off-by: Kaven Yau <love29881460@qq.com>

KavenYau · 2021-04-16T00:20:54Z

It's also related to #1599.

        if (!handle->is_active()) {
          return rclcpp_action::CancelResponse::REJECT;
        }
        return rclcpp_action::CancelResponse::ACCEPT;

if did not add the if condition to CancelCallback to check whether the goal active, it would throw exception.

Signed-off-by: Kaven Yau <love29881460@qq.com>

SteveMacenski · 2021-04-16T19:25:58Z

@clalancette I leave it to you if you think this bug patch should make it into Galactic, but it would be very nice if it did since it impacts a number of Nav2 users 😄

Love it, thanks for the patch @KavenYau! Hopefully this is the last-last race condition issue in the action server we'll need to fix 😄

clalancette · 2021-04-16T21:02:15Z

So the good news is that as it stands, this patch is ABI compatible. So we could do it in a Galactic patch release.

That said, this would be a good fix to have (and is one of the reasons to do testing at this point). So I've put it on the Galactic project board to look at. That doesn't guarantee we'll get it in, but we'll definitely consider it.

SteveMacenski · 2021-04-16T23:05:57Z

Let me know if I can help speed things up - its overall a pretty simple change so I'd hope a maintainer here could review it shortly

wjwwood

I'm concerned that this will require all cancel callbacks for servers to check the is_active() state... which isn't ideal, imo.

Also, I'm concerned we could be allowing a goal to change states from SUCCEEDED or ABORTED to CANCELED erroneously.

I don't feel confident approving this change myself, I think we need more eyes on this.

rclcpp_action/include/rclcpp_action/server.hpp

jacobperron

The change proposed in this PR looks okay to me. Correct me if I'm wrong, but I think the race related to transitioning to canceling and transitioning to a terminal state is not a new bug being introduced (it already exists).

rclcpp_action/include/rclcpp_action/server.hpp

KavenYau · 2021-04-21T22:29:20Z

Is there any progress on this？ 😁

fujitatomoya

this fix itself looks good to me.

fujitatomoya · 2021-04-22T04:08:24Z

@jacobperron @wjwwood it would be probably better if either of you take a look again? and about exception problem, i think we can discuss that on #1599?

wjwwood

changes lgtm

rclcpp_action/test/test_server.cpp

Co-authored-by: William Woodall <william+github@osrfoundation.org>

jacobperron

Linux
Linux-aarch64
macOS
Windows

fujitatomoya · 2021-04-23T08:42:41Z

it seems windows job got stuck to clear out docker containers?

00:00:09.572 C:\J\workspace\ci_windows>powershell -Command "if ($(docker ps -q) -ne $null) { docker stop $(docker ps -q)}" 
...

jacobperron · 2021-04-23T20:44:57Z

I've retriggered the Windows build.

fujitatomoya · 2021-04-25T07:32:26Z

@jacobperron thanks 👍

all green, i think this can be merged.

clalancette · 2021-04-26T16:51:13Z

Should we consider backporting this one to Galactic?

SteveMacenski · 2021-04-26T17:41:41Z

I think so 😄

Edit: Can this be backported to Foxy as well?

KavenYau · 2021-04-27T02:05:59Z

Should I backport this to Foxy / Galactic?

…in CancelCallback (ros2#1635)

clalancette · 2021-04-27T19:00:05Z

Should I backport this to Foxy / Galactic?

Definitely Galactic. I haven't looked at the code in Foxy, but if it applies there and doesn't break ABI, then it can be backported to Foxy as well.

jacobperron · 2021-04-27T19:28:48Z

I've already added this to the Foxy project board.

KavenYau · 2021-04-28T00:42:53Z

I've already added this to the Foxy project board.

Dose it mean that I don't need to make a backporting PR for it because it's already in plan?

clalancette · 2021-04-28T00:57:56Z

Dose it mean that I don't need to make a backporting PR for it because it's already in plan?

It still needs someone to do the work for both Galactic and Foxy, so if you are willing to do it, it would be appreciated.

ivanpauno · 2021-04-29T18:56:54Z

@Mergifyio backport galactic

…in CancelCallback (#1635) * Fix deadlock issue that caused by other mutexes locked in CancelCallback Signed-off-by: Kaven Yau <love29881460@qq.com> * Add unit test for rclcpp action server deadlock Signed-off-by: Kaven Yau <love29881460@qq.com> * Update rclcpp_action/test/test_server.cpp Co-authored-by: William Woodall <william+github@osrfoundation.org> Co-authored-by: Kaven Yau <love29881460@qq.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: William Woodall <william+github@osrfoundation.org> (cherry picked from commit fba080c)

mergify · 2021-04-29T18:57:09Z

Command backport galactic: success

Backports have been created

#1646 Fix action server deadlock issue that caused by other mutexes locked in CancelCallback (backport #1635) has been created for branch galactic

…in CancelCallback (#1635) (#1646) * Fix deadlock issue that caused by other mutexes locked in CancelCallback Signed-off-by: Kaven Yau <love29881460@qq.com> * Add unit test for rclcpp action server deadlock Signed-off-by: Kaven Yau <love29881460@qq.com> * Update rclcpp_action/test/test_server.cpp Co-authored-by: William Woodall <william+github@osrfoundation.org> Co-authored-by: Kaven Yau <love29881460@qq.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: William Woodall <william+github@osrfoundation.org> (cherry picked from commit fba080c) Co-authored-by: Kaven Yau <kavenyau@foxmail.com>

…in CancelCallback (ros2#1635) Signed-off-by: Kaven Yau <kavenyau@foxmail.com>

…in CancelCallback (#1635) (#1654) Signed-off-by: Kaven Yau <kavenyau@foxmail.com>

KavenYau changed the title ~~Fix action deadlock issue that caused by other mutexes locked in CancelCallback~~ Fix action server deadlock issue that caused by other mutexes locked in CancelCallback Apr 15, 2021

Fix deadlock issue that caused by other mutexes locked in CancelCallback

552a3a5

Signed-off-by: Kaven Yau <love29881460@qq.com>

KavenYau force-pushed the fix-goal_handle-race-condition-issue branch from 9fe631a to 9c76968 Compare April 16, 2021 00:02

KavenYau mentioned this pull request Apr 16, 2021

checking if handle is active in handle_cancel ros-navigation/navigation2#2304

Merged

6 tasks

KavenYau force-pushed the fix-goal_handle-race-condition-issue branch from 9c76968 to 4de34cd Compare April 16, 2021 01:41

Add unit test for rclcpp action server deadlock

cd78da4

Signed-off-by: Kaven Yau <love29881460@qq.com>

KavenYau force-pushed the fix-goal_handle-race-condition-issue branch from 4de34cd to cd78da4 Compare April 16, 2021 01:45

clalancette added this to In progress in Galactic via automation Apr 16, 2021

SteveMacenski approved these changes Apr 16, 2021

View reviewed changes

wjwwood reviewed Apr 17, 2021

View reviewed changes

rclcpp_action/include/rclcpp_action/server.hpp Show resolved Hide resolved

jacobperron reviewed Apr 19, 2021

View reviewed changes

rclcpp_action/include/rclcpp_action/server.hpp Show resolved Hide resolved

fujitatomoya approved these changes Apr 22, 2021

View reviewed changes

fujitatomoya mentioned this pull request Apr 22, 2021

node crashes while received a cancel request if the state is SUCCEEDED #1599

Closed

wjwwood reviewed Apr 22, 2021

View reviewed changes

rclcpp_action/test/test_server.cpp Show resolved Hide resolved

Update rclcpp_action/test/test_server.cpp

e1e1d69

Co-authored-by: William Woodall <william+github@osrfoundation.org>

jacobperron approved these changes Apr 22, 2021

View reviewed changes

jacobperron added this to Proposed in Foxy Patch Release 5 via automation Apr 22, 2021

fujitatomoya merged commit fba080c into ros2:master Apr 25, 2021

KavenYau mentioned this pull request Apr 26, 2021

Returns CancelResponse::REJECT while goal handle failed to transit to CANCELING state #1641

Merged

clalancette moved this from In progress to Done in Galactic Apr 26, 2021

KavenYau added a commit to KavenYau/rclcpp that referenced this pull request Apr 27, 2021

Fix action server deadlock issue that caused by other mutexes locked …

a6b0128

…in CancelCallback (ros2#1635)

mergify bot mentioned this pull request Apr 29, 2021

Fix action server deadlock issue that caused by other mutexes locked in CancelCallback (backport #1635) #1646

Merged

This was referenced Apr 30, 2021

Simple action server deadlock ros-navigation/navigation2#2273

Closed

goal_handle attempted invalid transition #1569

Closed

KavenYau added a commit to KavenYau/rclcpp that referenced this pull request May 2, 2021

Fix action server deadlock issue that caused by other mutexes locked …

64a377a

…in CancelCallback (ros2#1635) Signed-off-by: Kaven Yau <kavenyau@foxmail.com>

KavenYau mentioned this pull request May 2, 2021

[foxy backport #1635] Fix action server deadlock issue that caused by other mutexes locked in CancelCallback #1654

Merged

ivanpauno pushed a commit that referenced this pull request May 5, 2021

Fix action server deadlock issue that caused by other mutexes locked …

791c23a

…in CancelCallback (#1635) (#1654) Signed-off-by: Kaven Yau <kavenyau@foxmail.com>

jacobperron removed this from Proposed in Foxy Patch Release 5 Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix action server deadlock issue that caused by other mutexes locked in CancelCallback #1635

Fix action server deadlock issue that caused by other mutexes locked in CancelCallback #1635

KavenYau commented Apr 15, 2021

KavenYau commented Apr 16, 2021 •

edited

Loading

SteveMacenski commented Apr 16, 2021

clalancette commented Apr 16, 2021

SteveMacenski commented Apr 16, 2021

wjwwood left a comment

jacobperron left a comment

KavenYau commented Apr 21, 2021

fujitatomoya left a comment

fujitatomoya commented Apr 22, 2021

wjwwood left a comment

jacobperron left a comment •

edited

Loading

fujitatomoya commented Apr 23, 2021

jacobperron commented Apr 23, 2021

fujitatomoya commented Apr 25, 2021

clalancette commented Apr 26, 2021

SteveMacenski commented Apr 26, 2021 •

edited

Loading

KavenYau commented Apr 27, 2021 •

edited

Loading

clalancette commented Apr 27, 2021

jacobperron commented Apr 27, 2021

KavenYau commented Apr 28, 2021

clalancette commented Apr 28, 2021

ivanpauno commented Apr 29, 2021

mergify bot commented Apr 29, 2021

Fix action server deadlock issue that caused by other mutexes locked in CancelCallback #1635

Fix action server deadlock issue that caused by other mutexes locked in CancelCallback #1635

Conversation

KavenYau commented Apr 15, 2021

KavenYau commented Apr 16, 2021 • edited Loading

SteveMacenski commented Apr 16, 2021

clalancette commented Apr 16, 2021

SteveMacenski commented Apr 16, 2021

wjwwood left a comment

Choose a reason for hiding this comment

jacobperron left a comment

Choose a reason for hiding this comment

KavenYau commented Apr 21, 2021

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Apr 22, 2021

wjwwood left a comment

Choose a reason for hiding this comment

jacobperron left a comment • edited Loading

Choose a reason for hiding this comment

fujitatomoya commented Apr 23, 2021

jacobperron commented Apr 23, 2021

fujitatomoya commented Apr 25, 2021

clalancette commented Apr 26, 2021

SteveMacenski commented Apr 26, 2021 • edited Loading

KavenYau commented Apr 27, 2021 • edited Loading

clalancette commented Apr 27, 2021

jacobperron commented Apr 27, 2021

KavenYau commented Apr 28, 2021

clalancette commented Apr 28, 2021

ivanpauno commented Apr 29, 2021

mergify bot commented Apr 29, 2021

KavenYau commented Apr 16, 2021 •

edited

Loading

jacobperron left a comment •

edited

Loading

SteveMacenski commented Apr 26, 2021 •

edited

Loading

KavenYau commented Apr 27, 2021 •

edited

Loading