Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jan 20, 2021
Merged

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

merged 3 commits into from
Jan 20, 2021

Conversation

daisukes
Copy link
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
@clalancette @daisukes
Add missing locking to the rclcpp_action::ServerBase. (ros2#1421)
80c15f9 
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>
@daisukes
[backport] Fix action server deadlock (ros2#1285, ros2#1313)
83a45b5 
Signed-off-by: Daisuke Sato <daisukes@cmu.edu>
fujitatomoya
fujitatomoya approved these changes Jan 17, 2021
View reviewed changes
Copy link
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

@ivanpauno ivanpauno mentioned this pull request Jan 18, 2021
Closed
jacobperron
jacobperron reviewed Jan 19, 2021
View reviewed changes
Copy link
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.

rclcpp/include/rclcpp/experimental/intra_process_manager.hpp Outdated Show resolved Hide resolved
@daisukes
revert comment
fab2bfe 
Signed-off-by: Daisuke Sato <daisukes@cmu.edu>
jacobperron
jacobperron approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment •
edited
Loading

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)

@jacobperron jacobperron merged commit 6756ccf into ros2:foxy Jan 20, 2021
@KavenYau KavenYau mentioned this pull request Mar 27, 2021
Closed
@ros-discourse
Copy link

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
Reviewers

@jacobperron jacobperron jacobperron approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@daisukes @ros-discourse @jacobperron @fujitatomoya @clalancette