[19702] Fix Data Race when updating liveliness changed in WLP #3926

Mario-DL · 2023-10-11T12:38:38Z

Description

This PR fixes a data race in WLP::update_liveliness_changed_status() and repairs the Short liveliness Windows CI tests (see the manual job launch results here).

The PR is not included with a test since the same test suite of the ShortLivelinees (in windows) serves to demonstrate the issue

@Mergifyio backport 2.11.x 2.10.x 2.6.x

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
-N/A 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
N/A Any new/modified methods have been properly documented using Doxygen.
Changes are ABI compatible.
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.
Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

MiguelCompany

Let's see what TSan says about this. I remember this part of the code was subject to deadlocks in the past.

Besides that, see my suggestion below.

src/cpp/rtps/builtin/liveliness/WLP.cpp

Mario-DL · 2023-10-16T12:31:59Z

Temp fix moving the fastcdr branch to master was not enough for getting the tsan job to work as there are conflicts when merging from the forked repo.
Compiled in local with the following colcon cmake options.
Checked that correct gcc (12.3.0) was used by cmake in the compilation logs.
Exported the following variable in the terminal where the tests are run: export TSAN_OPTIONS=second_deadlock_stack=1 history_size=7 memory_limit_mb=5000
The following tests
BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Reliable.Transport BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Reliable.Intraprocess BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Reliable.Datasharing BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_BestEffort.Transport BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_BestEffort.Intraprocess BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_BestEffort.Datasharing BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Automatic_Reliable.Transport BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Automatic_Reliable.Intraprocess BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Automatic_Reliable.Datasharing BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Automatic_BestEffort.Transport BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Automatic_BestEffort.Intraprocess BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_Automatic_BestEffort.Datasharing BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_ManualByParticipant_Reliable.Transport BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_ManualByParticipant_Reliable.Intraprocess BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_ManualByParticipant_Reliable.Datasharing BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_ManualByParticipant_BestEffort.Transport BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_ManualByParticipant_BestEffort.Intraprocess BlackboxTests_DDS_PIM.LivelinessQos.ShortLiveliness_ManualByTopic_ManualByParticipant_BestEffort.Datasharing
passed without warnings nor errors.

MiguelCompany · 2023-10-16T12:58:04Z

@richiprosima Please test this

Mario-DL · 2023-10-18T08:31:31Z

Update & findings:

Running the thread sanitizer in Linux for BlackboxTests_FastRTPS.Discovery.TwentyParticipantsMulticast BlackboxTests_FastRTPS.Discovery.TwentyParticipantsMulticastLocalhostOnly BlackboxTests_FastRTPS.Discovery.TwentyParticipantsSeveralEndpointsMulticast BlackboxTests_FastRTPS.Discovery.TwentyParticipantsUnicast.Transport BlackboxTests_FastRTPS.Discovery.TwentyParticipantsUnicast.Intraprocess BlackboxTests_FastRTPS.Discovery.TwentyParticipantsSeveralEndpointsUnicast.Transport BlackboxTests_FastRTPS.Discovery.TwentyParticipantsSeveralEndpointsUnicast.Intraprocess BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsMulticast BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsMulticastLocalhostOnly BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsSeveralEndpointsMulticast BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsUnicast.Transport BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsUnicast.Intraprocess BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsUnicast.Datasharing BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsSeveralEndpointsUnicast.Transport BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsSeveralEndpointsUnicast.Intraprocess BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsSeveralEndpointsUnicast.Datasharing
Passes with no errors nor warnings. The BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsSeveralEndpointsUnicast.Datasharing was rerun with retest-until-fail 10 but already passed.
In Windows VM (4 cores), the BlackboxTests_DDS_PIM.Discovery.TwentyParticipantsSeveralEndpointsUnicast.Datasharing consistently fails (1 out of 10) in both master and fix/ci-github/short-liveliness-tests branches. Interestingly, it was observed that in both branches, sometimes fails by Timeout and sometimes due to the unhandled throw Failed init_port fastrtps_portXXXX: Interprocess mutex timeout when locking. Possible deadlock: owner died without unlocking? -> Function eprosima::fastdds::rtps::SharedMemGlobal::open_port_internal. This former exception sometimes appear during tests retries but in some occasions is not enough to make the test fail (in the code, another attempt of removing and creating the segment is performed before re-trowing the exception)

Hint: Increasing the timed_lock() waiting time to BOOST_INTERPROCESS_TIMEOUT_... * 5 seems to decrease the probability of the issue, but does not eliminate it

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

EduPonz · 2023-10-24T10:28:24Z

@richiprosima please test this

EduPonz · 2023-10-25T05:12:51Z

@richiprosima please test mac

Mario-DL · 2023-10-25T09:27:23Z

@richiprosima please test mac

Mario-DL · 2023-10-25T09:31:11Z

@richiprosima please test mac

Mario-DL · 2023-10-25T10:16:36Z

Regarding the linux failed tests. In local, compiled with TSAN, those same tests failures occur 1 every 30 attempts approx. TSAN does not output any warnings, the tests fail in the assert (std::find(triggered.begin(), triggered.end(), any_cond)) != (triggered.end()).
I could also reproduce the same test failures in master

Mario-DL · 2023-10-26T10:59:50Z

@Mergifyio backport 2.11.x 2.10.x 2.6.x

mergify · 2023-10-26T10:59:56Z

backport 2.11.x 2.10.x 2.6.x

✅ Backports have been created

#3959 [19702] Fix Data Race when updating liveliness changed in WLP (backport #3926) has been created for branch 2.11.x
#3960 [19702] Fix Data Race when updating liveliness changed in WLP (backport #3926) has been created for branch 2.10.x
#3961 [19702] Fix Data Race when updating liveliness changed in WLP (backport #3926) has been created for branch 2.6.x

* Refs #19702: Fix Data Race in WLP Signed-off-by: Mario Dominguez <[email protected]> * Refs #19702: reviewer suggestion Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> (cherry picked from commit 8126a51)

* Refs #19702: Fix Data Race in WLP Signed-off-by: Mario Dominguez <[email protected]> * Refs #19702: reviewer suggestion Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> (cherry picked from commit 8126a51) Co-authored-by: Mario Domínguez López <[email protected]>

Mario-DL added this to the v2.12.1 milestone Oct 11, 2023

Mario-DL added needs-review PR that is ready to be reviewed ci-pending PR which CI is running labels Oct 11, 2023

MiguelCompany requested changes Oct 11, 2023

View reviewed changes

src/cpp/rtps/builtin/liveliness/WLP.cpp Outdated Show resolved Hide resolved

Mario-DL removed the needs-review PR that is ready to be reviewed label Oct 16, 2023

Mario-DL force-pushed the fix/ci-github/short-liveliness-tests branch from 1a60361 to b558087 Compare October 16, 2023 12:22

MiguelCompany approved these changes Oct 16, 2023

View reviewed changes

Mario-DL added 2 commits October 24, 2023 09:16

Refs #19702: Fix Data Race in WLP

bd780ca

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

Refs #19702: reviewer suggestion

6c02a2b

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

Mario-DL force-pushed the fix/ci-github/short-liveliness-tests branch from b558087 to 6c02a2b Compare October 24, 2023 07:16

EduPonz removed the ci-pending PR which CI is running label Oct 26, 2023

EduPonz merged commit 8126a51 into master Oct 26, 2023
11 of 14 checks passed

EduPonz deleted the fix/ci-github/short-liveliness-tests branch October 26, 2023 08:01

eProsima deleted a comment from mergify bot Oct 26, 2023

mergify bot mentioned this pull request Oct 26, 2023

[19702] Fix Data Race when updating liveliness changed in WLP (backport #3926) #3959

Merged

mergify bot mentioned this pull request Oct 26, 2023

[19702] Fix Data Race when updating liveliness changed in WLP (backport #3926) #3960

Merged

mergify bot mentioned this pull request Oct 26, 2023

[19702] Fix Data Race when updating liveliness changed in WLP (backport #3926) #3961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[19702] Fix Data Race when updating liveliness changed in WLP #3926

[19702] Fix Data Race when updating liveliness changed in WLP #3926

Mario-DL commented Oct 11, 2023 •

edited by MiguelCompany

Loading

MiguelCompany left a comment

Mario-DL commented Oct 16, 2023 •

edited

Loading

MiguelCompany commented Oct 16, 2023

Mario-DL commented Oct 18, 2023 •

edited

Loading

EduPonz commented Oct 24, 2023

EduPonz commented Oct 25, 2023

Mario-DL commented Oct 25, 2023

Mario-DL commented Oct 25, 2023

Mario-DL commented Oct 25, 2023

Mario-DL commented Oct 26, 2023

mergify bot commented Oct 26, 2023 •

edited

Loading

[19702] Fix Data Race when updating liveliness changed in WLP #3926

[19702] Fix Data Race when updating liveliness changed in WLP #3926

Conversation

Mario-DL commented Oct 11, 2023 • edited by MiguelCompany Loading

Description

Contributor Checklist

Reviewer Checklist

MiguelCompany left a comment

Choose a reason for hiding this comment

Mario-DL commented Oct 16, 2023 • edited Loading

MiguelCompany commented Oct 16, 2023

Mario-DL commented Oct 18, 2023 • edited Loading

EduPonz commented Oct 24, 2023

EduPonz commented Oct 25, 2023

Mario-DL commented Oct 25, 2023

Mario-DL commented Oct 25, 2023

Mario-DL commented Oct 25, 2023

Mario-DL commented Oct 26, 2023

mergify bot commented Oct 26, 2023 • edited Loading

✅ Backports have been created

Mario-DL commented Oct 11, 2023 •

edited by MiguelCompany

Loading

Mario-DL commented Oct 16, 2023 •

edited

Loading

Mario-DL commented Oct 18, 2023 •

edited

Loading

mergify bot commented Oct 26, 2023 •

edited

Loading