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

[14813] WaitSet deadlock fix <master> #2744

Merged
merged 3 commits into from
Jun 17, 2022
Merged

[14813] WaitSet deadlock fix <master> #2744

merged 3 commits into from
Jun 17, 2022

Conversation

MiguelBarro
Copy link
Contributor

@MiguelBarro MiguelBarro commented Jun 10, 2022 •
edited
Loading

Signed-off-by: Javier Santiago javiersantiago@eprosima.com

The sanitizer ci runs & review on #2730
It must be merged after #2717

Description

This applies to Thread Sanitizer issues 4121, 4122, 4123.

All these issues follow the same pattern. A new Condition is registered while an old one is being destroyed. This causes a deadlock since the locks are taken as follows:

WaitSetImpl::attach_condition (A) -> ConditionNotifier::attach_to (B)
ConditionNotifier::will_be_deleted (B) -> WaitSetImpl::will_be_deleted (A)

We prevent the issue by reducing the scope of lock A's protection inside attach_condition and detach_condition.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added.
  • Any new/modified methods have been properly documented using Doxygen.
  • Fast DDS test suite has been run locally.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • Documentation builds and tests pass locally.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.

Reviewer Checklist

  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

Miguel Barro and others added 3 commits June 10, 2022 12:25
Refs 14681. Fixing issue 4169.
94c0453 
Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing compiler errors & warnings.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing testing Mocks.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Addressing reviewer comments.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Debugging fixes.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Linter pass

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Adding a ProxyPool class to avoid EDP ABBAs on temporary proxy mutex.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Prevent reentrancy issues on shared_mutexes using tls

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Mock updates to match new RTPSParticipantImpl behavior.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Linter pass..

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing gcc/clang warnings

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing clang warnings

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Unifying all temporary proxies in PDP owned pools

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Linter pass

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. RTPSStatisticsTests.statistics_rpts_listener_gap_callback fixed

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. RTPSWriterTests CMakelists.txt fix for windows.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing clang -Wunused-lambda-capture issue

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing proxies copy to fix PubSubBasic.ReceivedPartitionDataExceedsSizeLimit blackbox

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Addressing reviewer's comments

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
@jsan-rt
Refs #14813: WaitSet deadlock fix
2d9cb00 
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
@jsan-rt
Refs #14813: Added missing protection
183023f 
Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
@MiguelBarro MiguelBarro mentioned this pull request Jun 10, 2022
Merged
13 tasks
@EduPonz EduPonz added this to the v2.7.0 milestone Jun 16, 2022
@EduPonz EduPonz merged commit 7b05712 into master Jun 17, 2022
@EduPonz EduPonz deleted the test/issue/4121 branch June 17, 2022 05:47
@MiguelCompany
Copy link
Member

@Mergifyio backport 2.6.x

mergify bot pushed a commit that referenced this pull request Jun 22, 2022
@MiguelBarro @mergify
WaitSet deadlock fix (#2744)
fe4d7a4 
* Refs 14681. Fixing issue 4169.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing compiler errors & warnings.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing testing Mocks.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Addressing reviewer comments.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Debugging fixes.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Linter pass

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Adding a ProxyPool class to avoid EDP ABBAs on temporary proxy mutex.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Prevent reentrancy issues on shared_mutexes using tls

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Mock updates to match new RTPSParticipantImpl behavior.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Linter pass..

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing gcc/clang warnings

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing clang warnings

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Unifying all temporary proxies in PDP owned pools

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Linter pass

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. RTPSStatisticsTests.statistics_rpts_listener_gap_callback fixed

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. RTPSWriterTests CMakelists.txt fix for windows.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing clang -Wunused-lambda-capture issue

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Fixing proxies copy to fix PubSubBasic.ReceivedPartitionDataExceedsSizeLimit blackbox

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 14681. Addressing reviewer's comments

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

* Refs #14813: WaitSet deadlock fix

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Refs #14813: Added missing protection

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

Co-authored-by: Javier Santiago <javiersantiago@eprosima.com>
(cherry picked from commit 7b05712)
@mergify mergify bot mentioned this pull request Jun 22, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Jun 22, 2022

backport 2.6.x

✅ Backports have been created

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

@EduPonz EduPonz EduPonz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MiguelBarro @MiguelCompany @EduPonz @jsan-rt