[14833] DiscoveryServer list access deadlock fix <master> #2746

MiguelBarro · 2022-06-10T11:49:24Z

Review & thread sanitizer results on #2738
It must be merged after #2745

Description

This PR must be merged after #2730.

This PR fixes Thread Sanitizer issues 4135 and its equivalents.

This deadlock is caused by having the PDP lock taken when iterating over the DiscoveryServer list calling match_pdp_reader_nts_, which in turn calls matched_reader_add_, locking the Writer lock.
As seen in previous Sanitizer issues, these two locks should be taken in reverse order: Writer first then PDP. When a participant is enabled, it takes the mutexes in the proper order, leading to a deadlock.

The deadlock occurs as follows:

PDPServer::createPDPEndpoints (A) -> StatefulWriter::matched_reader_add (B)
PDPServer::announceParticipantState (B) -> PDPServer::announceParticipantState (A)

Since the second pair of locks cannot be reversed due to the previous rule. to prevent this issue the use of the PDP mutex is phased out in favor of a new shared_mutex inside the BuildinProtocols header to control access to DIscoveryServer list. This allows the replacement of some PDP locks by new shared_lock calls.

Collateral extra works

As a piggyback the FileWatch mechanism to load the Discovery Server List from a file was revamped.
This was unavoidable because some FileWatch related tests failed as a consequence of the synchronization changes.
To summarize the changes:

FileWatch detection granularity now matches the chrono::system_clock one in each OS.
FileWatch filtering of redundant callbacks has been improved using file size besides last write time.
An new test has been added to test a hotfix in the Discovery Server List update mechanism.

Contributor Checklist

Reviewer Checklist

Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

jsan-rt · 2022-06-15T20:49:44Z

@richiprosima please test this

jsan-rt · 2022-06-15T21:51:52Z

@richiprosima please test this again

jsan-rt · 2022-06-16T14:17:48Z

@richiprosima please test this yet again

jsan-rt · 2022-06-21T06:35:00Z

@richiprosima please test this

…cols DiscoveryServers list Signed-off-by: Javier Santiago <[email protected]>

Signed-off-by: Javier Santiago <[email protected]>

…oved incorrectly placed lock to the proper place. Signed-off-by: Javier Santiago <[email protected]>

Signed-off-by: Javier Santiago <[email protected]>

…ate announcement Signed-off-by: Javier Santiago <[email protected]>

Signed-off-by: Javier Santiago <[email protected]>

…cipant creation. Prevented wrong PDP access Signed-off-by: Javier Santiago <[email protected]>

Signed-off-by: Javier Santiago <[email protected]>

… test skeleton Signed-off-by: Javier Santiago <[email protected]>

Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro · 2022-06-23T14:16:05Z

@richiprosima Please test this for me 🤯

Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro · 2022-06-27T12:36:23Z

@richiprosima please test this

…objects. Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro · 2022-06-27T13:11:59Z

@richiprosima please test this

Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro · 2022-06-27T15:14:15Z

@richiprosima please test this

…ranularity Signed-off-by: Miguel Barro <[email protected]>

Signed-off-by: Miguel Barro <[email protected]>

…ation policy. Because under certain busy loads the callbacks may happen twice per file modification Signed-off-by: Miguel Barro <[email protected]>

Signed-off-by: Miguel Barro <[email protected]>

…ependent pull request Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro · 2022-06-28T14:07:48Z

@richiprosima please test this

Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro · 2022-06-28T14:16:35Z

@richiprosima please test this

MiguelCompany

Some minor comments.

I'm also a bit concerned that Discovery.TwentyParticipantsSeveralEndpointsUnicast tests have failed on several platforms.

include/fastdds/rtps/attributes/ServerAttributes.h

src/cpp/rtps/builtin/BuiltinProtocols.cpp

src/cpp/rtps/builtin/discovery/database/DiscoveryDataBase.cpp

src/cpp/rtps/builtin/discovery/participant/PDPServer.cpp

Signed-off-by: Miguel Barro <[email protected]>

MiguelCompany

LGTM

MiguelBarro · 2022-06-30T05:54:42Z

@richiprosima please test Mac. Let's see what happens with Discovery.TwentyParticipantsSeveralEndpointsUnicast

MiguelBarro · 2022-06-30T07:20:04Z

Discovery.TwentyParticipantsSeveralEndpointsUnicast passed in all its variants.

* Refs #14833: Added new shared mutex to control access to BuiltinProtocols DiscoveryServers list Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Cleanup Signed-off-by: Javier Santiago <[email protected]> * Reversed PDP and Writer lock order inside announceParticipantState. Moved incorrectly placed lock to the proper place. Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Linter pass Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Reversed lock order for PDP server/client participant state announcement Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Fixed Windows Warnings Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Applied suggestions. Improved Filewatch trigger on participant creation. Prevented wrong PDP access Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Restricted PDP lock scope Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Linter pass Signed-off-by: Javier Santiago <[email protected]> * Applied reviewer inline comments Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Fixed wrong addition to discovery server list. Added new test skeleton Signed-off-by: Javier Santiago <[email protected]> * Refs #14833: Fixing and Adding new tests. Fixing linux & mac warnings. Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Use specific filenames per test to allow concurrency Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Linter pass Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Fixing clang warnings Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Fixing clang warnings Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Fixing clang warnings Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Avoiding fix delay on FileWatch callbacks Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Linter Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Linter Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Fixing clang issues Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Low FileWatch granularity from seconds to system_clock one (Windows 100ns/Unix 1ns) Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Speed up ParticipantTests that involve file callbacks Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: trim tests CMakeLists.txt from sources exporting unused objects. Signed-off-by: Miguel Barro <[email protected]> * Refs #14833. partial review Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Fixing SystemInfoTests to account for the new filetime granularity Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Avoid using chrono literals (c++14) Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Account for stat() delay to retrieve actual FileTime info. Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: modify the test to prevent the strict 1 call per modification policy. Because under certain busy loads the callbacks may happen twice per file modification Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Linter pass Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Improve FileWatch callback filtering using file size Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: New callback filter makes waits redundant Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Undo DiscoveryDatabase fix for a more complete on an independent pull request Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Fixing clang warnings Signed-off-by: Miguel Barro <[email protected]> * Refs #14833: Addressing reviewers' comments Signed-off-by: Miguel Barro <[email protected]> Co-authored-by: Javier Santiago <[email protected]> Co-authored-by: Miguel Barro <[email protected]>

MiguelBarro added the skip-ci Automatically pass CI label Jun 10, 2022

jsan-rt force-pushed the test/issue/4135 branch 3 times, most recently from efcbe21 to cd2e0c8 Compare June 14, 2022 09:05

jsan-rt removed the skip-ci Automatically pass CI label Jun 15, 2022

EduPonz added this to the v2.7.0 milestone Jun 16, 2022

jsan-rt force-pushed the test/issue/4135 branch from 4c50233 to f751769 Compare June 21, 2022 06:18

MiguelBarro force-pushed the test/issue/4135 branch 2 times, most recently from 98db776 to f02e71e Compare June 23, 2022 13:05

jsan-rt and others added 14 commits June 23, 2022 16:13

Refs #14833: Added new shared mutex to control access to BuiltinProto…

7e2d57d

…cols DiscoveryServers list Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Cleanup

6529750

Signed-off-by: Javier Santiago <[email protected]>

Reversed PDP and Writer lock order inside announceParticipantState. M…

3cdb1e0

…oved incorrectly placed lock to the proper place. Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Linter pass

9a4aad6

Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Reversed lock order for PDP server/client participant st…

af2140c

…ate announcement Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Fixed Windows Warnings

ffa500d

Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Applied suggestions. Improved Filewatch trigger on parti…

aea795d

…cipant creation. Prevented wrong PDP access Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Restricted PDP lock scope

a1075f0

Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Linter pass

7315dcb

Signed-off-by: Javier Santiago <[email protected]>

Applied reviewer inline comments

28fcf2e

Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Fixed wrong addition to discovery server list. Added new…

0cd87fb

… test skeleton Signed-off-by: Javier Santiago <[email protected]>

Refs #14833: Fixing and Adding new tests. Fixing linux & mac warnings.

05f0350

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Use specific filenames per test to allow concurrency

33e3e07

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Linter pass

8e5772c

Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro force-pushed the test/issue/4135 branch from c20fe2a to 8e5772c Compare June 23, 2022 14:15

Refs #14833: Fixing clang warnings

e95e717

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Speed up ParticipantTests that involve file callbacks

6b93a6c

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: trim tests CMakeLists.txt from sources exporting unused …

984f6d2

…objects. Signed-off-by: Miguel Barro <[email protected]>

Refs #14833. partial review

19ac062

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Fixing SystemInfoTests to account for the new filetime g…

f11fe5f

…ranularity Signed-off-by: Miguel Barro <[email protected]>

MiguelBarro force-pushed the test/issue/4135 branch from cdc152b to f11fe5f Compare June 28, 2022 08:50

Miguel Barro added 7 commits June 28, 2022 11:13

Refs #14833: Avoid using chrono literals (c++14)

d42721f

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Account for stat() delay to retrieve actual FileTime info.

b066a86

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: modify the test to prevent the strict 1 call per modific…

f9ceea6

…ation policy. Because under certain busy loads the callbacks may happen twice per file modification Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Linter pass

3bb78a2

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Improve FileWatch callback filtering using file size

9d4ac55

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: New callback filter makes waits redundant

fd5c68b

Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Undo DiscoveryDatabase fix for a more complete on an ind…

a18c110

…ependent pull request Signed-off-by: Miguel Barro <[email protected]>

Refs #14833: Fixing clang warnings

d2f6136

Signed-off-by: Miguel Barro <[email protected]>

MiguelCompany requested changes Jun 29, 2022

View reviewed changes

Refs #14833: Addressing reviewers' comments

571d2aa

Signed-off-by: Miguel Barro <[email protected]>

MiguelCompany approved these changes Jun 29, 2022

View reviewed changes

MiguelCompany merged commit 7a2f319 into master Jun 30, 2022

MiguelCompany deleted the test/issue/4135 branch June 30, 2022 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[14833] DiscoveryServer list access deadlock fix <master> #2746

[14833] DiscoveryServer list access deadlock fix <master> #2746

MiguelBarro commented Jun 10, 2022 •

edited by MiguelCompany

Loading

jsan-rt commented Jun 15, 2022

jsan-rt commented Jun 15, 2022

jsan-rt commented Jun 16, 2022

jsan-rt commented Jun 21, 2022

MiguelBarro commented Jun 23, 2022

MiguelBarro commented Jun 27, 2022

MiguelBarro commented Jun 27, 2022

MiguelBarro commented Jun 27, 2022

MiguelBarro commented Jun 28, 2022

MiguelBarro commented Jun 28, 2022 •

edited

Loading

MiguelCompany left a comment

MiguelCompany left a comment

MiguelBarro commented Jun 30, 2022

MiguelBarro commented Jun 30, 2022

[14833] DiscoveryServer list access deadlock fix <master> #2746

[14833] DiscoveryServer list access deadlock fix <master> #2746

Conversation

MiguelBarro commented Jun 10, 2022 • edited by MiguelCompany Loading

Description

Collateral extra works

Contributor Checklist

Reviewer Checklist

jsan-rt commented Jun 15, 2022

jsan-rt commented Jun 15, 2022

jsan-rt commented Jun 16, 2022

jsan-rt commented Jun 21, 2022

MiguelBarro commented Jun 23, 2022

MiguelBarro commented Jun 27, 2022

MiguelBarro commented Jun 27, 2022

MiguelBarro commented Jun 27, 2022

MiguelBarro commented Jun 28, 2022

MiguelBarro commented Jun 28, 2022 • edited Loading

MiguelCompany left a comment

Choose a reason for hiding this comment

MiguelCompany left a comment

Choose a reason for hiding this comment

MiguelBarro commented Jun 30, 2022

MiguelBarro commented Jun 30, 2022

MiguelBarro commented Jun 10, 2022 •

edited by MiguelCompany

Loading

MiguelBarro commented Jun 28, 2022 •

edited

Loading