Skip to content

Commit

Permalink
api_integrationtest: Fix TSAN data race during shutdown
Browse files Browse the repository at this point in the history
TSAN reported the problem below on a CI run.

There's a possible sequence of events that can lead to this race (I'm
not sure it's the only one):
* The perfetto SDK thread calls the OnStop callback.
* The perfetto SDK thread is now inside WaitableTestEvent::Notify().
* The perfetto SDK thread sets notified_ to true and it unlocks the
  mutex, but does not call `cv_.notify_one()`.
* The main thread executes `WaitableTestEvent::Wait()`, which at this
  point can return.
* The main thread finishes executing the test and destroys the tracing
  sessions, which destroys the WaitableTestEvent and therefore `cv_`.
* The perfetto SDK thread resumes execution and tries to execute
  `cv_.notify_one()`, but that races with `cv_` destructor.

In order to prevent this race, we can just call `cv_.notify_one()` under
the lock. It might be less efficient, but it's correct.

This reverts part of https://r.android.com/1373304

https://ci.perfetto.dev/#!/logs/20221219192758--cls-2345694-4--linux-clang-x86_64-tsan

```
[00:07:40] [ RUN      ] PerfettoApiTest/PerfettoApiTest.TrackEventObserver_TwoDataSources/System
[00:07:40] [901.194] ing_service_impl.cc:980 Configured tracing session 1, #sources:1, duration:500 ms, #buffers:1, total buffer size:1024 KB, total sessions:1, uid:1337 session name: ""
[00:07:41] [901.697] ng_service_impl.cc:1921 FlushAndDisableTracing(1) done, success=1
[00:07:41] [901.698] ing_service_impl.cc:980 Configured tracing session 2, #sources:1, duration:500 ms, #buffers:1, total buffer size:1024 KB, total sessions:2, uid:1337 session name: ""
[00:07:41] [902.200] ng_service_impl.cc:1921 FlushAndDisableTracing(2) done, success=1
[00:07:42] ==================
[00:07:42] WARNING: ThreadSanitizer: data race (pid=7841)
[00:07:42]   Write of size 8 at 0x7b2000019c40 by main thread:
[00:07:42]     #0 pthread_cond_destroy ??:? (perfetto_integrationtests+0x315f8d) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#1 std::Cr::condition_variable::~condition_variable() ??:? (libc++.so+0xad729) (BuildId: bea413d45487d5c2)
[00:07:42]     catapult-project#2 (anonymous namespace)::PerfettoApiTest::TearDown() api_integrationtest.cc:? (perfetto_integrationtests+0x398609) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#3 testing::Test::Run() ??:? (perfetto_integrationtests+0x8de94f) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#4 testing::TestInfo::Run() ??:? (perfetto_integrationtests+0x8e0a2c) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#5 testing::TestSuite::Run() ??:? (perfetto_integrationtests+0x8e1b14) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#6 testing::internal::UnitTestImpl::RunAllTests() ??:? (perfetto_integrationtests+0x8f9e3f) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#7 testing::UnitTest::Run() ??:? (perfetto_integrationtests+0x8f8d36) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#8 main api_integrationtest_main.cc:? (perfetto_integrationtests+0x4d7aa4) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]
[00:07:42]   Previous read of size 8 at 0x7b2000019c40 by thread T3:
[00:07:42]     #0 pthread_cond_signal ??:? (perfetto_integrationtests+0x315c28) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#1 std::Cr::condition_variable::notify_one() ??:? (libc++.so+0xad479) (BuildId: bea413d45487d5c2)
[00:07:42]     catapult-project#2 void std::Cr::__function::__policy_invoker<void ()>::__call_impl<std::Cr::__function::__default_alloc_func<(anonymous namespace)::PerfettoApiTest::NewTrace(perfetto::protos::gen::TraceConfig const&, perfetto::BackendType, int)::{lambda()catapult-project#1}, void ()> >(std::Cr::__function::__policy_storage const*) api_integrationtest.cc:? (perfetto_integrationtests+0x39ba0a) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#3 perfetto::base::UnixTaskRunner::RunImmediateAndDelayedTask() unix_task_runner.cc:? (perfetto_integrationtests+0x594cfd) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#4 perfetto::base::UnixTaskRunner::Run() unix_task_runner.cc:? (perfetto_integrationtests+0x594133) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#5 perfetto::base::ThreadTaskRunner::RunTaskThread(std::Cr::function<void (perfetto::base::UnixTaskRunner*)>) thread_task_runner.cc:? (perfetto_integrationtests+0x592e4f) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#6 void* std::Cr::__thread_proxy[abi:v160000]<std::Cr::tuple<std::Cr::unique_ptr<std::Cr::__thread_struct, std::Cr::default_delete<std::Cr::__thread_struct> >, void (perfetto::base::ThreadTaskRunner::*)(std::Cr::function<void (perfetto::base::UnixTaskRunner*)>), perfetto::base::ThreadTaskRunner*, std::Cr::function<void (perfetto::base::UnixTaskRunner*)> > >(void*) thread_task_runner.cc:? (perfetto_integrationtests+0x59380b) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]
[00:07:42]   Location is heap block of size 120 at 0x7b2000019c00 allocated by main thread:
[00:07:42]     #0 operator new(unsigned long) ??:? (perfetto_integrationtests+0x3941a7) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#1 (anonymous namespace)::PerfettoApiTest::NewTrace(perfetto::protos::gen::TraceConfig const&, int) api_integrationtest.cc:? (perfetto_integrationtests+0x39b864) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#2 (anonymous namespace)::PerfettoApiTest::NewTraceWithCategories(std::Cr::vector<std::Cr::basic_string<char, std::Cr::char_traits<char>, std::Cr::allocator<char> >, std::Cr::allocator<std::Cr::basic_string<char, std::Cr::char_traits<char>, std::Cr::allocator<char> > > >, perfetto::protos::gen::TrackEventConfig, perfetto::protos::gen::TraceConfig) api_integrationtest.cc:? (perfetto_integrationtests+0x3a6385) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#3 (anonymous namespace)::PerfettoApiTest_TrackEventObserver_TwoDataSources_Test::TestBody() api_integrationtest.cc:? (perfetto_integrationtests+0x4a61d2) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#4 testing::Test::Run() ??:? (perfetto_integrationtests+0x8de8c3) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#5 testing::TestInfo::Run() ??:? (perfetto_integrationtests+0x8e0a2c) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#6 testing::TestSuite::Run() ??:? (perfetto_integrationtests+0x8e1b14) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#7 testing::internal::UnitTestImpl::RunAllTests() ??:? (perfetto_integrationtests+0x8f9e3f) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#8 testing::UnitTest::Run() ??:? (perfetto_integrationtests+0x8f8d36) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     #9 main api_integrationtest_main.cc:? (perfetto_integrationtests+0x4d7aa4) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]
[00:07:42]   Thread T3 'TracingMuxer' (tid=7845, running) created by main thread at:
[00:07:42]     #0 pthread_create ??:? (perfetto_integrationtests+0x3147ab) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#1 perfetto::base::ThreadTaskRunner::ThreadTaskRunner(std::Cr::basic_string<char, std::Cr::char_traits<char>, std::Cr::allocator<char> > const&) thread_task_runner.cc:? (perfetto_integrationtests+0x592b80) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#2 perfetto::(anonymous namespace)::PlatformPosix::CreateTaskRunner(perfetto::Platform::CreateTaskRunnerArgs const&) platform_posix.cc:? (perfetto_integrationtests+0x8c524a) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#3 perfetto::internal::TracingMuxerImpl::TracingMuxerImpl(perfetto::TracingInitArgs const&) tracing_muxer_impl.cc:? (perfetto_integrationtests+0x5a731c) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#4 perfetto::internal::TracingMuxerImpl::InitializeInstance(perfetto::TracingInitArgs const&) tracing_muxer_impl.cc:? (perfetto_integrationtests+0x5ad7fb) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#5 perfetto::Tracing::InitializeInternal(perfetto::TracingInitArgs const&) tracing.cc:? (perfetto_integrationtests+0x5c0f0d) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#6 (anonymous namespace)::ConcurrentSessionTest_ConcurrentBackends_Test::TestBody() api_integrationtest.cc:? (perfetto_integrationtests+0x4c3af4) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#7 testing::Test::Run() ??:? (perfetto_integrationtests+0x8de8c3) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     catapult-project#8 testing::TestInfo::Run() ??:? (perfetto_integrationtests+0x8e0a2c) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     #9 testing::TestSuite::Run() ??:? (perfetto_integrationtests+0x8e1b14) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     #10 testing::internal::UnitTestImpl::RunAllTests() ??:? (perfetto_integrationtests+0x8f9e3f) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     #11 testing::UnitTest::Run() ??:? (perfetto_integrationtests+0x8f8d36) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]     #12 main api_integrationtest_main.cc:? (perfetto_integrationtests+0x4d7aa4) (BuildId: 2d1df55c4bda2cdd)
[00:07:42]
[00:07:42] SUMMARY: ThreadSanitizer: data race ??:? in __interceptor_pthread_cond_destroy
[00:07:42] ==================
```

Change-Id: Ic822dba258b81a6c032633cf63763bd578739024
  • Loading branch information
ddiproietto committed Jan 12, 2023
1 parent 20cc6d2 commit 17e91c4
Showing 1 changed file with 2 additions and 6 deletions.
8 changes: 2 additions & 6 deletions src/tracing/test/api_integrationtest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,8 @@ class WaitableTestEvent {
}

void Notify() {
{
std::lock_guard<std::mutex> lock(mutex_);
notified_ = true;
}
// Do not notify while holding the lock, because then we wake up the other
// end, only for it to fail to acquire the lock.
std::lock_guard<std::mutex> lock(mutex_);
notified_ = true;
cv_.notify_one();
}

Expand Down

0 comments on commit 17e91c4

Please sign in to comment.