Skip to content

[Backport] Fix action server deadlock (#1285) (#1313) #1518

Merged
jacobperron merged 3 commits intoros2:foxyfrom
daisukes:foxy-deadlock-fix-backport
Jan 20, 2021
Merged

[Backport] Fix action server deadlock (#1285) (#1313) #1518
jacobperron merged 3 commits intoros2:foxyfrom
daisukes:foxy-deadlock-fix-backport

Conversation

@daisukes
Copy link
Copy Markdown
Contributor

This is a backport of #1313
I didn't include 8c8c268 and 543a3c3, but 579e9d0 and modified ca3ad7d.

clalancette and others added 2 commits January 15, 2021 13:44
This patch actually does 4 related things:

1.  Renames the recursive mutex in the ServerBaseImpl class
to action_server_reentrant_mutex_, which makes it a lot
clearer what it is meant to lock.
2.  Adds some additional error checking where checks were missed.
3.  Adds a lock to publish_status so that the action_server
structure is protected.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Daisuke Sato <daisukes@cmu.edu>
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending CI. One nitpick that I'd like to see the change to the comment in rclcpp reverted.

Signed-off-by: Daisuke Sato <daisukes@cmu.edu>
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status (unrelated warnings)

@ros-discourse
Copy link
Copy Markdown

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/april-foxy-sync/19851/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants