Fix few tsan issues #16710

MBkkt · 2022-08-04T11:04:25Z

Scope & Purpose

💩 Bugfix
🍕 New feature
🔥 Performance improvement
🔨 Refactoring/simplification

Checklist

Related Information

(Please reference tickets / specification / other PRs etc)

Docs PR:
Enterprise PR:
GitHub issue / Jira ticket:
Design document:

MBkkt · 2022-08-04T18:29:14Z

tests/Replication2/ModelChecker/ModelChecker.h

      threads.emplace_back([&driver, initialObserver, initialState, iters,
                            threadSeed = gen(), result = &results[thrIdx]] {
-        // the random device is thread_local
-        RandomGenerator::initialize(RandomGenerator::RandomType::MERSENNE);


RandomGenerator::initialize reset thread_local ptr to null and global enum type to mersenne

So this code no make sense, because

we have data race on type and we can init it before threads creation.

thread_local ptr initialized by null on thread creation, this thread created right here

MBkkt · 2022-08-04T18:40:20Z

arangod/Aql/ClusterQuery.cpp

@@ -27,6 +27,7 @@
 #include "Aql/AqlTransaction.h"
 #include "Aql/ExecutionEngine.h"
 #include "Aql/Timing.h"
+#include "Aql/QueryCache.h"


Some magic (don't understand how it works before), but I can't build without it (incomplete type), change only shared_ptr creation

MBkkt · 2022-08-04T18:41:25Z

arangod/Aql/QueryOptions.h

@@ -62,6 +62,8 @@ enum class TraversalProfileLevel : uint8_t {
 struct QueryOptions {
  QueryOptions();
  explicit QueryOptions(arangodb::velocypack::Slice);
+  QueryOptions(QueryOptions&&) noexcept = default;


It moved in some place but didn't have move :(

MBkkt · 2022-08-04T18:42:13Z

tsan_arangodb_suppressions.txt

+# TODO Fix data race in arangodump
+race:arangodump
+
+# TODO Fix data race in arangorestore
+race:arangorestore


A lot of races in random code

MBkkt · 2022-08-04T18:43:11Z

tsan_arangodb_suppressions.txt

+deadlock:consensus::Agent::setPersistedState
+
+# TODO Fix data race in arangodbtests
+race:DummyConnection::sendRequest


It's stupid race, difficult to fix because need to correct tests
Now them written bad

MBkkt · 2022-08-04T18:44:13Z

tsan_arangodb_suppressions.txt

+thread:CacheManagerFeature::start
+
+# TODO Fix lock order inversion
+deadlock:consensus::Agent::setPersistedState


Should be fixed by cluster team.
"Agent call State" but sometimes "State call Agent"

MBkkt · 2022-08-04T18:45:26Z

tsan_arangodb_suppressions.txt

+# TODO Fix known thread leaks
+thread:ClusterFeature::startHeartbeatThread
+thread:CacheManagerFeature::start


Should be fixed by cluster team
Also not critical them leaked after stop application

MBkkt · 2022-08-08T07:39:20Z

lib/Futures/Future.h

-  std::mutex m;
-  std::condition_variable cv;
-


It is bug. What if timeout and thenFinal callback still not called

MBkkt · 2022-08-08T07:42:03Z

lib/Futures/Future.h

-  FutureStatus wait_until(
-      const std::chrono::time_point<Clock, Duration>& timeout_time) {
-    detail::waitImpl(*this, timeout_time);
-    return FutureStatus::Ready;


MBkkt · 2022-08-08T07:42:26Z

lib/Futures/Future.h

-  /// waits for the result, returns if it is not available
-  /// for the specified timeout duration. Future must be valid
-  template<class Rep, class Period>
-  FutureStatus wait_for(
-      const std::chrono::duration<Rep, Period>& timeout_duration) {
-    return wait_until(std::chrono::steady_clock::now() + timeout_duration);
-  }
-
-  /// waits for the result, returns if it is not available until
-  /// specified time point. Future must be valid
-  template<class Clock, class Duration>
-  FutureStatus wait_until(
-      const std::chrono::time_point<Clock, Duration>& timeout_time) {
-    detail::waitImpl(*this, timeout_time);
-    return FutureStatus::Ready;


used only in itself tests

MBkkt · 2022-08-08T07:42:48Z

tests/Fuerte/ConnectionFailuresTest.cpp

@@ -51,7 +51,7 @@ static void tryToConnectExpectFailure(f::EventLoopService& eventLoopService,
                          [&](f::Error, std::unique_ptr<f::Request>,
                              std::unique_ptr<f::Response>) {});

-  auto success = wg.wait_for(std::chrono::seconds(5));


failed locally for vst

MBkkt · 2022-08-08T07:53:02Z

https://jenkins01.arangodb.biz/job/arangodb-ANY-linux-san.x86-64/1216/EDITION=enterprise,SAN_MODE=TSan,STORAGE_ENGINE=rocksdb,TEST_SUITE=single,limit=linux&&test&&x86-64/

js/client/modules/@arangodb/testutils/test-utils.js

arangod/Aql/Query.cpp

arangod/Aql/ClusterQuery.cpp

mpoeter · 2022-08-08T14:26:28Z

arangod/Aql/ClusterQuery.cpp

+      try {
+        _traversers.clear();
+      } catch (...) {
+      }


IMHO this should be kept in the ClusterQuery dtor as before (then we can also keep the traversers private).

arangod/Aql/Query.cpp

tsan_arangodb_suppressions.txt

…e path

MBkkt · 2022-08-09T10:27:26Z

https://jenkins01.arangodb.biz/job/arangodb-ANY-linux-san.x86-64/

(cherry picked from commit af11ab0)

* Fix few tsan issues (#16710) (cherry picked from commit af11ab0) * Fix data race on query vptr * Update arangod/Aql/Query.h Co-authored-by: Jan <jsteemann@users.noreply.github.com> * Apply suggestions from code review * Destroy query before destroy traverses Co-authored-by: Jan <jsteemann@users.noreply.github.com> Co-authored-by: Vadim Kondratyev <vadim@arangodb.com>

MBkkt force-pushed the bugfix/tsan-issue-query branch 2 times, most recently from fe7bc07 to 1a23c28 Compare August 4, 2022 11:37

gnusi added the TSan label Aug 4, 2022

mpoeter self-requested a review August 4, 2022 13:31

MBkkt requested a review from jsteemann August 4, 2022 17:33

MBkkt force-pushed the bugfix/tsan-issue-query branch from 1a23c28 to 5ddaa6b Compare August 4, 2022 18:17

MBkkt commented Aug 4, 2022

View reviewed changes

MBkkt marked this pull request as ready for review August 4, 2022 18:38

MBkkt requested a review from a team as a code owner August 4, 2022 18:38

MBkkt commented Aug 4, 2022

View reviewed changes

MBkkt force-pushed the bugfix/tsan-issue-query branch 3 times, most recently from 4df7985 to 7bc64a2 Compare August 6, 2022 12:03

MBkkt commented Aug 8, 2022

View reviewed changes

MBkkt requested a review from gnusi August 8, 2022 07:47

MBkkt changed the title ~~Bugfix/tsan issue query~~ Fix few tsan issues Aug 8, 2022

mpoeter suggested changes Aug 8, 2022

View reviewed changes

js/client/modules/@arangodb/testutils/test-utils.js Outdated Show resolved Hide resolved

arangod/Aql/Query.cpp Outdated Show resolved Hide resolved

arangod/Aql/ClusterQuery.cpp Outdated Show resolved Hide resolved

MBkkt requested a review from mpoeter August 8, 2022 12:15

mpoeter reviewed Aug 8, 2022

View reviewed changes

arangod/Aql/ClusterQuery.cpp Outdated Show resolved Hide resolved

MBkkt requested a review from mpoeter August 8, 2022 13:42

mpoeter reviewed Aug 8, 2022

View reviewed changes

MBkkt added 23 commits August 9, 2022 13:16

Try to fix data race on query vptr

ed03bbd

Fix data race, RandomGenerator is thread_local, but its type not

3dc9036

Suppress known thread leaks

5e9cd19

Suppress known lock inversion

77969bf

Suppress arangodump and arangorestore data races

6307ff1

Try to fix tsan symbolize problem by skip -noasan tests

066157b

Fix another data race in tests

6c33189

Fix another data race in tests

5b37cd2

Fix race between condvar dtor and notify

e151a57

Fix suppression boost lockfree queue for case when we cannot symboliz…

1415cee

…e path

Fix slow test

10397d4

Suppress thread leak

d00e127

Fix wait with timeout in arangodb::Future

6001556

Fix suppression

53607f9

Remove unused and incorrect part of future api

b7e123b

Fix tsan suppression for case without file in stacktrace

cbccfd1

Fix tsan suppression for case without file in stacktrace

9bd425b

temp

e9f7935

Fix by Manuel suggestion

e106f91

Fix by Manuel suggestion

5afd52a

Fix by Manuel suggestion

8391356

temp

9040363

temp

556d501

MBkkt force-pushed the bugfix/tsan-issue-query branch from 35fbf4f to 556d501 Compare August 9, 2022 10:18

mpoeter approved these changes Aug 10, 2022

View reviewed changes

jsteemann merged commit af11ab0 into devel Aug 10, 2022

jsteemann deleted the bugfix/tsan-issue-query branch August 10, 2022 09:59

MBkkt added a commit that referenced this pull request Aug 11, 2022

Fix few tsan issues (#16710)

a386a5a

(cherry picked from commit af11ab0)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix few tsan issues #16710

Fix few tsan issues #16710

MBkkt commented Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022

MBkkt Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022

MBkkt Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022

MBkkt Aug 8, 2022

MBkkt Aug 8, 2022

MBkkt Aug 8, 2022

MBkkt Aug 8, 2022

MBkkt commented Aug 8, 2022

mpoeter Aug 8, 2022

MBkkt Aug 8, 2022

MBkkt Aug 9, 2022

MBkkt commented Aug 9, 2022

Fix few tsan issues #16710

Fix few tsan issues #16710

Conversation

MBkkt commented Aug 4, 2022 • edited Loading

Scope & Purpose

Checklist

Related Information

MBkkt Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

MBkkt Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBkkt Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBkkt Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBkkt commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBkkt commented Aug 9, 2022

MBkkt commented Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022 •

edited

Loading

MBkkt Aug 4, 2022 •

edited

Loading