Skip to content

[21670] Fix Data Races on DDS-Pipe#145

Merged
juanlofer-eprosima merged 9 commits into
mainfrom
use_debug_for_tsan
Jul 14, 2025
Merged

[21670] Fix Data Races on DDS-Pipe#145
juanlofer-eprosima merged 9 commits into
mainfrom
use_debug_for_tsan

Conversation

@cferreiragonz

@cferreiragonz cferreiragonz commented May 27, 2025

Copy link
Copy Markdown
Contributor

This PR is a follow up of eProsima/DDS-Router#509 to fix tsan tests of the DDS Router. It fixes two data races:

  • First, it uses the enabled_ atomic boolean of the BaseReader to ensure that the on_data_available_lambda_ is only called when the class has been properly enabled, that is, when the lambda has already been set. This change is not mandatory, but TSAN stops reporting a false positive related to a data race with the the mentioned lambda .
  • Then, it uses DDS/Rtps Listeners as attributes rather than inheriting from them. In this way, a data race between the on_participant_discovery callback and the destructor is avoided (data race on vptr (ctor/dtor vs virtual call)). This data rece occurs when a thread makes a call to a virtual method ("dds.udp" calls listener_->on_participant_discovery) while at the same time the destructor of the object that owns the virtual method is called (~CommonParticipant). Calling set_listener(nullptr) within the destructor does not prevent the data race, because the vtable is modified in the prologue of the destructor.
  • It also prevents several data races by avoiding calling set_listener after the Reader initialization.

@codecov-commenter

codecov-commenter commented May 27, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 31.94444% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.96%. Comparing base (d0008a6) to head (2277ab2).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
...ants/src/cpp/participant/dds/CommonParticipant.cpp 42.30% 3 Missing and 12 partials ⚠️
.../participant/dynamic_types/DynTypesParticipant.cpp 0.00% 12 Missing ⚠️
...nts/src/cpp/participant/rtps/CommonParticipant.cpp 47.82% 3 Missing and 9 partials ⚠️
...e_participants/src/cpp/reader/dds/CommonReader.cpp 12.50% 6 Missing and 1 partial ⚠️
.../participant/dynamic_types/DynTypesParticipant.hpp 0.00% 2 Missing ⚠️
...articipants/src/cpp/reader/auxiliar/BaseReader.cpp 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
+ Coverage   36.49%   37.96%   +1.46%     
==========================================
  Files         170      154      -16     
  Lines       11657     7020    -4637     
  Branches     5327     2788    -2539     
==========================================
- Hits         4254     2665    -1589     
+ Misses       4645     2954    -1691     
+ Partials     2758     1401    -1357     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread ddspipe_participants/src/cpp/participant/dds/CommonParticipant.cpp Outdated
Comment thread ddspipe_participants/src/cpp/reader/auxiliar/BaseReader.cpp
Comment thread ddspipe_participants/src/cpp/reader/dds/CommonReader.cpp
Comment thread ddspipe_participants/src/cpp/reader/rtps/CommonReader.cpp
@cferreiragonz

Copy link
Copy Markdown
Contributor Author

As internally agreed, I have disabled the auto-enabling of the DataReader at DDS level and now we manually enable it. This is not mandatory tho, it only protects against highly unlikely changes in future Fast DDS releases.

Comment thread ddspipe_participants/src/cpp/participant/dds/CommonParticipant.cpp Outdated
Comment thread ddspipe_participants/src/cpp/participant/dds/CommonParticipant.cpp
Comment thread ddspipe_participants/src/cpp/participant/dds/CommonParticipant.cpp
Comment thread ddspipe_participants/src/cpp/participant/rtps/CommonParticipant.cpp
Comment thread ddspipe_participants/src/cpp/reader/dds/CommonReader.cpp
Comment thread ddspipe_participants/src/cpp/reader/dds/CommonReader.cpp
Comment thread ddspipe_participants/src/cpp/reader/dds/CommonReader.cpp
Comment thread ddspipe_participants/src/cpp/reader/rtps/CommonReader.cpp
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
@cferreiragonz

Copy link
Copy Markdown
Contributor Author

Revision applied on commit 71631b6. I also rebased on top of master

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

@juanlofer-eprosima juanlofer-eprosima left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@juanlofer-eprosima juanlofer-eprosima merged commit ee0e639 into main Jul 14, 2025
15 checks passed
@juanlofer-eprosima juanlofer-eprosima deleted the use_debug_for_tsan branch July 14, 2025 05:20
@cferreiragonz

Copy link
Copy Markdown
Contributor Author

@Mergifyio backport 0.x

@mergify

mergify Bot commented Jul 14, 2025

Copy link
Copy Markdown

backport 0.x

✅ Backports have been created

Details

mergify Bot pushed a commit that referenced this pull request Jul 14, 2025
* Refs #21670: Protect on_data_available_lambda_

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Set listener in RederCreation

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Separate DDS/RTPS Listeners from Base Classes

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Uncrustify

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply Review

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply Review 2

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply review 3

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Avoid using capital letters in DDS Listener

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Fix leak

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

---------

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
(cherry picked from commit ee0e639)

# Conflicts:
#	ddspipe_participants/include/ddspipe_participants/participant/dds/CommonParticipant.hpp
#	ddspipe_participants/include/ddspipe_participants/participant/dynamic_types/DynTypesParticipant.hpp
#	ddspipe_participants/include/ddspipe_participants/participant/rtps/CommonParticipant.hpp
#	ddspipe_participants/src/cpp/participant/dds/CommonParticipant.cpp
#	ddspipe_participants/src/cpp/participant/dynamic_types/DynTypesParticipant.cpp
#	ddspipe_participants/src/cpp/participant/rtps/CommonParticipant.cpp
#	ddspipe_participants/src/cpp/reader/rtps/CommonReader.cpp
cferreiragonz added a commit that referenced this pull request Jul 14, 2025
* Refs #21670: Protect on_data_available_lambda_

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Set listener in RederCreation

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Separate DDS/RTPS Listeners from Base Classes

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Uncrustify

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply Review

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply Review 2

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply review 3

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Avoid using capital letters in DDS Listener

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Fix leak

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

---------

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
(cherry picked from commit ee0e639)
juanlofer-eprosima pushed a commit that referenced this pull request Jul 15, 2025
* Fix Data Races on DDS-Pipe (#145)

* Refs #21670: Protect on_data_available_lambda_

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Set listener in RederCreation

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Separate DDS/RTPS Listeners from Base Classes

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Uncrustify

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply Review

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply Review 2

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Apply review 3

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Avoid using capital letters in DDS Listener

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

* Refs #21670: Fix leak

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

---------

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
(cherry picked from commit ee0e639)

* Fix conflicts

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

---------

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
Co-authored-by: Carlos Ferreira González <carloos.499@gmail.com>
Co-authored-by: cferreiragonz <carlosferreira@eprosima.com>
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