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.

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

Fix deadlock between PDP and StatefulReader #871

Merged
merged 2 commits into from
Nov 21, 2019

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Nov 19, 2019

This is intended to fix a deadlock that causes tests to hang on the ROS 2 buildfarm (ros2/build_farmer#248) using Fast-RTPS v1.9.2. There's more info here: ros2/build_farmer#248 (comment).

This PR avoids deadlock by releasing the PDP mutex before calling remove_remote_participant(). It seems like a reasonable thing to do since remove_remote_participant() acquires the lock internally, though I don't know if releasing the lock momentarily breaks any assumptions in check_remote_participant_lifeliness().

This releases the PDP mutex before calling a function that may try to
acquire the mutex on a StatefulReader.

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Nov 19, 2019

@richiprosima would it be possible to get this reviewed and merged into the 1.9.x branch this week?

@dirk-thomas @mjcarroll FYI

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status
  • Performance Build Status

Copy link
Member

@richiware richiware left a comment

Choose a reason for hiding this comment

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

I agree with the PR. Only one suggestion.

src/cpp/rtps/builtin/discovery/participant/PDP.cpp Outdated Show resolved Hide resolved
@richiware richiware changed the base branch from master to 1.9.x November 20, 2019 15:25
@richiware richiware changed the base branch from 1.9.x to master November 20, 2019 15:25
@richiware
Copy link
Member

After running the CI I will merge the PR and tomorrow I will make the backport to 1.9.x

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status
  • Performance Build Status

@richiware
Copy link
Member

Failed test not related.

@richiware richiware merged commit 1ad4e63 into eProsima:master Nov 21, 2019
richiware pushed a commit that referenced this pull request Nov 21, 2019
* Fix deadlock between PDP and StatefulReader

This releases the PDP mutex before calling a function that may try to
acquire the mutex on a StatefulReader.

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>

* Removing unnecessary lock().
richiware added a commit that referenced this pull request Nov 21, 2019
Fix deadlock between PDP and StatefulReader (backport of #871)
sloretz added a commit to sloretz/Fast-RTPS that referenced this pull request Feb 19, 2020
* Fix deadlock between PDP and StatefulReader

This releases the PDP mutex before calling a function that may try to
acquire the mutex on a StatefulReader.

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>

* Removing unnecessary lock().

Signed-off-by: Shane Loretz <[email protected]>
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.

3 participants