[21293] Fix destruction data-race on participant removal in intra-process #5034

Mario-DL · 2024-07-07T21:00:12Z

Description

This PR addresses a race issue happening in stressed intraprocess scenarios when EDP's writer intends to use the remote local reader pointer of an already removed participant. This happens because the participant hasn't received the other's one disposal yet (as it goes through transport).

Some ci flaky tests have already been identified to be related with this issue.

The proposed solution introduces a new state in the Readers LocalReaderViewStatus in which the reader will notify that it is inactive as soon as it is destroyed and noone is using it.
On the other side, the remote local writers using pointers to it, now holds a LocalReaderPointer which wraps the raw reader's pointer plus the view. An internal counter now accounts for the number of references.

Thanks @MiguelCompany for helping with the final's solution design.

Note: the test may be launched with --restest-until-fail 20 or so, in order to reproduce the issue. For a more frequent failure, review can launch the colcon test with the taskset -c 0,1 prefix to make the test to stress more and make it fail more frequently.

@Mergifyio backport 3.1.x 3.0.x 2.14.x 2.10.x

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; the added tests pass locally
Any new/modified methods have been properly documented using Doxygen.
N/A Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
Changes are backport compatible: they do NOT break ABI nor change library core behavior.
Changes are API compatible.
N/A New feature has been added to the versions.md file (if applicable).
N/A New feature has been documented/Current behavior is correctly described in the documentation.
Applicable backports have been included in the description.

Reviewer Checklist

The PR has a milestone assigned.
The title and description correctly express the PR's purpose.
Check contributor checklist is correct.
If this is a critical bug fix, backports to the critical-only supported branches have been requested.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

Signed-off-by: Mario Dominguez <[email protected]>

…calReaderPointer> and properly calls local_actions_on_reader_removed() Signed-off-by: Mario Dominguez <[email protected]>

…when accessing local reader Signed-off-by: Mario Dominguez <[email protected]>

Signed-off-by: Mario Dominguez <[email protected]>

MiguelCompany

LGTM with green CI 🏅

MiguelCompany · 2024-10-29T13:31:06Z

@Mergifyio backport 3.1.x 3.0.x 2.14.x

mergify · 2024-10-29T13:31:36Z

backport 3.1.x 3.0.x 2.14.x

✅ Backports have been created

#5365 [21293] Fix destruction data-race on participant removal in intra-process (backport #5034) has been created for branch 3.1.x
#5366 [21293] Fix destruction data-race on participant removal in intra-process (backport #5034) has been created for branch 3.0.x
#5367 [21293] Fix destruction data-race on participant removal in intra-process (backport #5034) has been created for branch 2.14.x but encountered conflicts

) * Refs #21293: Add BB test Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Reinforce test to fail more frequently Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add RefCountedPointer.hpp to utils Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add unittests for RefCountedPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: LocalReaderPointer.hpp Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: BaseReader aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: ReaderLocator aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSDomainImpl::find_local_reader returns a sared_ptr<LocalReaderPointer> and properly calls local_actions_on_reader_removed() Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSWriters properly using LocalReaderPointer::Instance when accessing local reader Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Linter Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Fix windows warnings Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Address Miguel's review Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Apply last comment Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: NIT Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> (cherry picked from commit 456e45f)

) * Refs #21293: Add BB test Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Reinforce test to fail more frequently Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add RefCountedPointer.hpp to utils Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add unittests for RefCountedPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: LocalReaderPointer.hpp Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: BaseReader aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: ReaderLocator aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSDomainImpl::find_local_reader returns a sared_ptr<LocalReaderPointer> and properly calls local_actions_on_reader_removed() Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSWriters properly using LocalReaderPointer::Instance when accessing local reader Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Linter Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Fix windows warnings Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Address Miguel's review Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Apply last comment Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: NIT Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> (cherry picked from commit 456e45f) # Conflicts: # include/fastdds/rtps/writer/ReaderLocator.h # include/fastdds/rtps/writer/ReaderProxy.h # src/cpp/rtps/RTPSDomain.cpp # src/cpp/rtps/RTPSDomainImpl.hpp # src/cpp/rtps/participant/RTPSParticipantImpl.cpp # src/cpp/rtps/participant/RTPSParticipantImpl.h # src/cpp/rtps/reader/BaseReader.cpp # src/cpp/rtps/reader/BaseReader.hpp # src/cpp/rtps/writer/ReaderLocator.cpp # src/cpp/rtps/writer/StatefulWriter.cpp # src/cpp/rtps/writer/StatelessWriter.cpp # test/blackbox/common/DDSBlackboxTestsBasic.cpp # test/mock/rtps/ReaderLocator/fastdds/rtps/writer/ReaderLocator.h # test/unittest/utils/CMakeLists.txt

src/cpp/rtps/participant/RTPSParticipantImpl.cpp

) (#5366) * Refs #21293: Add BB test Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Reinforce test to fail more frequently Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add RefCountedPointer.hpp to utils Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add unittests for RefCountedPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: LocalReaderPointer.hpp Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: BaseReader aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: ReaderLocator aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSDomainImpl::find_local_reader returns a sared_ptr<LocalReaderPointer> and properly calls local_actions_on_reader_removed() Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSWriters properly using LocalReaderPointer::Instance when accessing local reader Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Linter Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Fix windows warnings Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Address Miguel's review Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Apply last comment Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: NIT Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> (cherry picked from commit 456e45f) Co-authored-by: Mario Domínguez López <[email protected]>

) (#5365) * Refs #21293: Add BB test Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Reinforce test to fail more frequently Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add RefCountedPointer.hpp to utils Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Add unittests for RefCountedPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: LocalReaderPointer.hpp Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: BaseReader aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: ReaderLocator aggregates LocalReaderPointer Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSDomainImpl::find_local_reader returns a sared_ptr<LocalReaderPointer> and properly calls local_actions_on_reader_removed() Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: RTPSWriters properly using LocalReaderPointer::Instance when accessing local reader Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Linter Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Fix windows warnings Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Address Miguel's review Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: Apply last comment Signed-off-by: Mario Dominguez <[email protected]> * Refs #21293: NIT Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> (cherry picked from commit 456e45f) Co-authored-by: Mario Domínguez López <[email protected]>

Mario-DL added this to the v3.0.0 milestone Jul 7, 2024

Mario-DL force-pushed the hotfix/21293 branch from bd4c638 to 18b7f62 Compare July 9, 2024 14:54

EduPonz modified the milestones: v3.0.0, v3.0.1 Jul 19, 2024

Mario-DL modified the milestones: v3.0.1, v3.0.2 Sep 4, 2024

MiguelCompany modified the milestones: v3.0.2, v3.1.0 Oct 2, 2024

rsanchez15 modified the milestones: v3.1.0, v3.1.1 Oct 3, 2024

Mario-DL force-pushed the hotfix/21293 branch from f435614 to be38120 Compare October 3, 2024 10:47

Mario-DL marked this pull request as ready for review October 3, 2024 11:06

Mario-DL requested review from richiprosima and removed request for richiprosima October 3, 2024 11:07

github-actions bot added the ci-pending PR which CI is running label Oct 3, 2024

Mario-DL requested review from richiprosima and removed request for richiprosima October 3, 2024 11:10

Mario-DL force-pushed the hotfix/21293 branch from a489d24 to 4962d18 Compare October 3, 2024 20:07

Mario-DL requested review from richiprosima and removed request for richiprosima October 3, 2024 20:08

Mario-DL force-pushed the hotfix/21293 branch from 4962d18 to b14868d Compare October 3, 2024 21:39

Mario-DL requested review from richiprosima and removed request for richiprosima October 3, 2024 21:40

MiguelCompany self-requested a review October 11, 2024 09:25

Mario-DL force-pushed the hotfix/21293 branch from b14868d to 91ac4b7 Compare October 14, 2024 16:40

Mario-DL requested review from MiguelCompany and removed request for MiguelCompany October 14, 2024 16:41

Mario-DL added 11 commits October 28, 2024 15:32

Refs #21293: Add unittests for RefCountedPointer

6670fa3

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: LocalReaderPointer.hpp

51d8731

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: BaseReader aggregates LocalReaderPointer

a4f3b9b

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: ReaderLocator aggregates LocalReaderPointer

bc5e09e

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: RTPSDomainImpl::find_local_reader returns a sared_ptr<Lo…

2ff1831

…calReaderPointer> and properly calls local_actions_on_reader_removed() Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: RTPSWriters properly using LocalReaderPointer::Instance …

781779c

…when accessing local reader Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: Linter

413d08f

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: Fix windows warnings

8413ada

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: Address Miguel's review

af900a9

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: Apply last comment

e0502da

Signed-off-by: Mario Dominguez <[email protected]>

Refs #21293: NIT

df98301

Signed-off-by: Mario Dominguez <[email protected]>

Mario-DL force-pushed the hotfix/21293 branch from 700bb79 to df98301 Compare October 28, 2024 14:33

Mario-DL requested review from MiguelCompany and removed request for MiguelCompany October 28, 2024 14:34

MiguelCompany approved these changes Oct 28, 2024

View reviewed changes

MiguelCompany added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Oct 29, 2024

MiguelCompany merged commit 456e45f into master Oct 29, 2024
17 checks passed

MiguelCompany deleted the hotfix/21293 branch October 29, 2024 13:30

mergify bot mentioned this pull request Oct 29, 2024

[21293] Fix destruction data-race on participant removal in intra-process (backport #5034) #5365

Merged

13 tasks

This was referenced Oct 29, 2024

[21293] Fix destruction data-race on participant removal in intra-process (backport #5034) #5366

Merged

[21293] Fix destruction data-race on participant removal in intra-process (backport #5034) #5367

Open

richiware requested changes Oct 29, 2024

View reviewed changes

src/cpp/rtps/participant/RTPSParticipantImpl.cpp Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[21293] Fix destruction data-race on participant removal in intra-process #5034

[21293] Fix destruction data-race on participant removal in intra-process #5034

Mario-DL commented Jul 7, 2024 •

edited by MiguelCompany

Loading

MiguelCompany left a comment

MiguelCompany commented Oct 29, 2024

mergify bot commented Oct 29, 2024 •

edited

Loading

[21293] Fix destruction data-race on participant removal in intra-process #5034

[21293] Fix destruction data-race on participant removal in intra-process #5034

Conversation

Mario-DL commented Jul 7, 2024 • edited by MiguelCompany Loading

Description

Contributor Checklist

Reviewer Checklist

MiguelCompany left a comment

Choose a reason for hiding this comment

MiguelCompany commented Oct 29, 2024

mergify bot commented Oct 29, 2024 • edited Loading

✅ Backports have been created

Mario-DL commented Jul 7, 2024 •

edited by MiguelCompany

Loading

mergify bot commented Oct 29, 2024 •

edited

Loading