Skip to content

Conversation

@mergify
Copy link

@mergify mergify bot commented Jul 14, 2025

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.
    This is an automatic backport of pull request [21670] Fix Data Races on DDS-Pipe #145 done by Mergify.

@mergify mergify bot added the conflicts label Jul 14, 2025
@mergify
Copy link
Author

mergify bot commented Jul 14, 2025

Cherry-pick of ee0e639 has failed:

On branch mergify/bp/0.x/pr-145
Your branch is up to date with 'origin/0.x'.

You are currently cherry-picking commit ee0e639.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   ddspipe_participants/src/cpp/reader/auxiliar/BaseReader.cpp
	modified:   ddspipe_participants/src/cpp/reader/dds/CommonReader.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   ddspipe_participants/include/ddspipe_participants/participant/dds/CommonParticipant.hpp
	both modified:   ddspipe_participants/include/ddspipe_participants/participant/dynamic_types/DynTypesParticipant.hpp
	both modified:   ddspipe_participants/include/ddspipe_participants/participant/rtps/CommonParticipant.hpp
	both modified:   ddspipe_participants/src/cpp/participant/dds/CommonParticipant.cpp
	both modified:   ddspipe_participants/src/cpp/participant/dynamic_types/DynTypesParticipant.cpp
	both modified:   ddspipe_participants/src/cpp/participant/rtps/CommonParticipant.cpp
	both modified:   ddspipe_participants/src/cpp/reader/rtps/CommonReader.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

cferreiragonz and others added 2 commits July 14, 2025 08:53
* 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)
Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 31.94444% with 49 lines in your changes missing coverage. Please review.

Please upload report for BASE (0.x@8abd633). Learn more about missing BASE report.

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           @@
##             0.x     #152   +/-   ##
======================================
  Coverage       ?   34.11%           
======================================
  Files          ?      155           
  Lines          ?     7903           
  Branches       ?     3517           
======================================
  Hits           ?     2696           
  Misses         ?     3283           
  Partials       ?     1924           

☔ 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.

@cferreiragonz
Copy link
Contributor

cferreiragonz commented Jul 14, 2025

Tools CI with this branch:

Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima left a comment

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 edde48f into 0.x Jul 15, 2025
15 of 17 checks passed
@juanlofer-eprosima juanlofer-eprosima deleted the mergify/bp/0.x/pr-145 branch July 15, 2025 05:50
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.

4 participants