Skip to content

[23767] Fix data races on DataWriterImpl qos access#6409

Open
ZakariaTalbi wants to merge 4 commits into
masterfrom
tsan/data_writers_qos
Open

[23767] Fix data races on DataWriterImpl qos access#6409
ZakariaTalbi wants to merge 4 commits into
masterfrom
tsan/data_writers_qos

Conversation

@ZakariaTalbi

@ZakariaTalbi ZakariaTalbi commented May 21, 2026 •
edited by MiguelCompany
Loading

Copy link
Copy Markdown
Contributor

Description

This PR attempts to fix data races on DataWriterImpl::qos_, where mainly the enable() and set_qos functions accessed the element simultaneously.

Added a new qos_mutex_ that guards qos_ and writer_. A concurrency regression test has also been included.

@Mergifyio backport 3.2.x 2.14.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.
  • N/A: 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.

@ZakariaTalbi ZakariaTalbi added this to the v3.7.0 milestone May 21, 2026
@ZakariaTalbi ZakariaTalbi marked this pull request as draft May 21, 2026 14:21
@ZakariaTalbi ZakariaTalbi changed the title Tsan/data writers qos [WIP] Tsan/data writers qos May 21, 2026
@ZakariaTalbi ZakariaTalbi changed the title [WIP] Tsan/data writers qos [WIP] Fix data racer on DataWriterImpl qos access May 21, 2026
@ZakariaTalbi ZakariaTalbi changed the title [WIP] Fix data racer on DataWriterImpl qos access [WIP] Fix data races on DataWriterImpl qos access May 21, 2026
@ZakariaTalbi ZakariaTalbi changed the title [WIP] Fix data races on DataWriterImpl qos access [WIP] #23767 Fix data races on DataWriterImpl qos access May 21, 2026
@ZakariaTalbi ZakariaTalbi requested a review from richiprosima May 21, 2026 14:23
@github-actions github-actions Bot added the ci-pending PR which CI is running label May 21, 2026
@ZakariaTalbi ZakariaTalbi requested review from richiprosima and removed request for richiprosima May 22, 2026 06:18
@ZakariaTalbi ZakariaTalbi changed the title [WIP] #23767 Fix data races on DataWriterImpl qos access [23767] Fix data races on DataWriterImpl qos access (WIP) May 22, 2026
@ZakariaTalbi
Test for Concurrent Writer Qos Access
b68fca8 
Signed-off-by: Zakaria Talbi Lalmi <zakariatalbi@eprosima.com>
@ZakariaTalbi
Access Synchronization for Writer Qos
1625c77 
Signed-off-by: Zakaria Talbi Lalmi <zakariatalbi@eprosima.com>
@ZakariaTalbi
Changes on the name of inner lock variable.
a2250d5 
Signed-off-by: Zakaria Talbi Lalmi <zakariatalbi@eprosima.com>
@ZakariaTalbi ZakariaTalbi force-pushed the tsan/data_writers_qos branch from ef0a012 to a2250d5 Compare May 25, 2026 09:49
@ZakariaTalbi ZakariaTalbi requested review from richiprosima and removed request for richiprosima May 25, 2026 09:49
@ZakariaTalbi ZakariaTalbi changed the title [23767] Fix data races on DataWriterImpl qos access (WIP) [23767] Fix data races on DataWriterImpl qos access May 26, 2026
@MiguelCompany MiguelCompany modified the milestones: v3.7.0, v3.6.2 May 27, 2026
@MiguelCompany MiguelCompany marked this pull request as ready for review May 27, 2026 08:49
@MiguelCompany MiguelCompany removed the request for review from richiprosima May 27, 2026 08:49
MiguelCompany
MiguelCompany requested changes May 27, 2026
View reviewed changes
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
Comment thread src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated
@MiguelCompany MiguelCompany self-requested a review May 28, 2026 08:10
@ZakariaTalbi
Applied suggestions on DataWriterImpl.
b0e8cb8 
Signed-off-by: Zakaria Talbi Lalmi <zakariatalbi@eprosima.com>
@ZakariaTalbi ZakariaTalbi force-pushed the tsan/data_writers_qos branch from f8b3c69 to b0e8cb8 Compare May 28, 2026 14:53
@MiguelCompany MiguelCompany requested review from MiguelCompany and removed request for MiguelCompany June 1, 2026 07:30
@MiguelCompany MiguelCompany removed the request for review from richiprosima June 5, 2026 10:49
MiguelCompany
MiguelCompany reviewed Jun 8, 2026
View reviewed changes

@MiguelCompany MiguelCompany left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have not found the data race mentioned in the description inside the reports of the TSAN nightly jobs. This means that this PR needs a regression test

@ZakariaTalbi

ZakariaTalbi commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

I have not found the data race mentioned in the description inside the reports of the TSAN nightly jobs. This means that this PR needs a regression test

I want to believe ConcurrentSetQosVsGetQos already tests the data race when run with TSAN meta.

@MiguelCompany

MiguelCompany commented Jun 8, 2026

Copy link
Copy Markdown
Member

I want to believe ConcurrentSetQosVsGetQos already tests the data race when run with TSAN meta.

You are right, it should!
I launched TSAN on the first commit of this PR here. Let's see if the data races are reported.

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

Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees

No one assigned

Labels

ci-pending PR which CI is running

Projects

None yet

Milestone

v3.6.2

Development

Successfully merging this pull request may close these issues.

2 participants

@ZakariaTalbi @MiguelCompany