zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix a data race in the thread unit tests.

Browse Source 
The flag used in thread_unittest.cc:FunctorB is subject to a (mostly
harmless) data race. In a tsan build, reproduce using

  out/Release/rtc_unittests --gtest_filter=AsyncInvokeTest.FireAndForget

There are additional tsan warnings, not all deterministic, when
running all the rtc_unittets: Some data races related to destructors,
and a locking-order-inversion warning. Hence applying this patch does
not make the unit tests tsan-clean.

I should also add that this is my very first cl, so I'm not at all
familiar with the process.

Review URL: https://codereview.webrtc.org/1439613004

Cr-Commit-Position: refs/heads/master@{#10645}
This commit is contained in:
nisse
2015-11-16 00:54:07 -08:00
committed by Commit bot
parent 6f14be8df8
commit d9b75bef5d
1 changed files with 71 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

110
webrtc/base/thread_unittest.cc
View File

@ -137,16 +137,48 @@ class SignalWhenDestroyedThread : public Thread {
Event* event_; Event* event_;
}; };
// A bool wrapped in a mutex, to avoid data races. Using a volatile
// bool should be sufficient for correct code ("eventual consistency"
// between caches is sufficient), but we can't tell the compiler about
// that, and then tsan complains about a data race.
// See also discussion at
// http://stackoverflow.com/questions/7223164/is-mutex-needed-to-synchronize-a-simple-flag-between-pthreads
// Using std::atomic<bool> or std::atomic_flag in C++11 is probably
// the right thing to do, but those features are not yet allowed. Or
// rtc::AtomicInt, if/when that is added. Since the use isn't
// performance critical, use a plain critical section for the time
// being.
class AtomicBool {
public:
explicit AtomicBool(bool value = false) : flag_(value) {}
AtomicBool& operator=(bool value) {
CritScope scoped_lock(&cs_);
flag_ = value;
return *this;
}
bool get() const {
CritScope scoped_lock(&cs_);
return flag_;
}
private:
mutable CriticalSection cs_;
bool flag_;
};
// Function objects to test Thread::Invoke. // Function objects to test Thread::Invoke.
struct FunctorA { struct FunctorA {
int operator()() { return 42; } int operator()() { return 42; }
}; };
class FunctorB { class FunctorB {
public: public:
explicit FunctorB(bool* flag) : flag_(flag) {} explicit FunctorB(AtomicBool* flag) : flag_(flag) {}
void operator()() { if (flag_) *flag_ = true; } void operator()() { if (flag_) *flag_ = true; }
private: private:
bool* flag_; AtomicBool* flag_;
}; };
struct FunctorC { struct FunctorC {
int operator()() { int operator()() {
@ -266,10 +298,10 @@ TEST(ThreadTest, Invoke) {
thread.Start(); thread.Start();
// Try calling functors. // Try calling functors.
EXPECT_EQ(42, thread.Invoke<int>(FunctorA())); EXPECT_EQ(42, thread.Invoke<int>(FunctorA()));
bool called = false; AtomicBool called;
FunctorB f2(&called); FunctorB f2(&called);
thread.Invoke<void>(f2); thread.Invoke<void>(f2);
EXPECT_TRUE(called); EXPECT_TRUE(called.get());
// Try calling bare functions. // Try calling bare functions.
struct LocalFuncs { struct LocalFuncs {
static int Func1() { return 999; } static int Func1() { return 999; }
@ -408,9 +440,9 @@ TEST_F(AsyncInvokeTest, FireAndForget) {
Thread thread; Thread thread;
thread.Start(); thread.Start();
// Try calling functor. // Try calling functor.
bool called = false; AtomicBool called;
invoker.AsyncInvoke<void>(&thread, FunctorB(&called)); invoker.AsyncInvoke<void>(&thread, FunctorB(&called));
EXPECT_TRUE_WAIT(called, kWaitTimeout); EXPECT_TRUE_WAIT(called.get(), kWaitTimeout);
} }
TEST_F(AsyncInvokeTest, WithCallback) { TEST_F(AsyncInvokeTest, WithCallback) {
@ -478,26 +510,26 @@ TEST_F(AsyncInvokeTest, KillInvokerBeforeExecute) {
TEST_F(AsyncInvokeTest, Flush) { TEST_F(AsyncInvokeTest, Flush) {
AsyncInvoker invoker; AsyncInvoker invoker;
bool flag1 = false; AtomicBool flag1;
bool flag2 = false; AtomicBool flag2;
// Queue two async calls to the current thread. // Queue two async calls to the current thread.
invoker.AsyncInvoke<void>(Thread::Current(), invoker.AsyncInvoke<void>(Thread::Current(),
FunctorB(&flag1)); FunctorB(&flag1));
invoker.AsyncInvoke<void>(Thread::Current(), invoker.AsyncInvoke<void>(Thread::Current(),
FunctorB(&flag2)); FunctorB(&flag2));
// Because we haven't pumped messages, these should not have run yet. // Because we haven't pumped messages, these should not have run yet.
EXPECT_FALSE(flag1); EXPECT_FALSE(flag1.get());
EXPECT_FALSE(flag2); EXPECT_FALSE(flag2.get());
// Force them to run now. // Force them to run now.
invoker.Flush(Thread::Current()); invoker.Flush(Thread::Current());
EXPECT_TRUE(flag1); EXPECT_TRUE(flag1.get());
EXPECT_TRUE(flag2); EXPECT_TRUE(flag2.get());
} }
TEST_F(AsyncInvokeTest, FlushWithIds) { TEST_F(AsyncInvokeTest, FlushWithIds) {
AsyncInvoker invoker; AsyncInvoker invoker;
bool flag1 = false; AtomicBool flag1;
bool flag2 = false; AtomicBool flag2;
// Queue two async calls to the current thread, one with a message id. // Queue two async calls to the current thread, one with a message id.
invoker.AsyncInvoke<void>(Thread::Current(), invoker.AsyncInvoke<void>(Thread::Current(),
FunctorB(&flag1), FunctorB(&flag1),
@ -505,17 +537,17 @@ TEST_F(AsyncInvokeTest, FlushWithIds) {
invoker.AsyncInvoke<void>(Thread::Current(), invoker.AsyncInvoke<void>(Thread::Current(),
FunctorB(&flag2)); FunctorB(&flag2));
// Because we haven't pumped messages, these should not have run yet. // Because we haven't pumped messages, these should not have run yet.
EXPECT_FALSE(flag1); EXPECT_FALSE(flag1.get());
EXPECT_FALSE(flag2); EXPECT_FALSE(flag2.get());
// Execute pending calls with id == 5. // Execute pending calls with id == 5.
invoker.Flush(Thread::Current(), 5); invoker.Flush(Thread::Current(), 5);
EXPECT_TRUE(flag1); EXPECT_TRUE(flag1.get());
EXPECT_FALSE(flag2); EXPECT_FALSE(flag2.get());
flag1 = false; flag1 = false;
// Execute all pending calls. The id == 5 call should not execute again. // Execute all pending calls. The id == 5 call should not execute again.
invoker.Flush(Thread::Current()); invoker.Flush(Thread::Current());
EXPECT_FALSE(flag1); EXPECT_FALSE(flag1.get());
EXPECT_TRUE(flag2); EXPECT_TRUE(flag2.get());
} }
class GuardedAsyncInvokeTest : public testing::Test { class GuardedAsyncInvokeTest : public testing::Test {
@ -564,11 +596,11 @@ TEST_F(GuardedAsyncInvokeTest, KillThreadFireAndForget) {
// Kill |thread|. // Kill |thread|.
thread = nullptr; thread = nullptr;
// Try calling functor. // Try calling functor.
bool called = false; AtomicBool called;
EXPECT_FALSE(invoker->AsyncInvoke<void>(FunctorB(&called))); EXPECT_FALSE(invoker->AsyncInvoke<void>(FunctorB(&called)));
// With thread gone, nothing should happen. // With thread gone, nothing should happen.
WAIT(called, kWaitTimeout); WAIT(called.get(), kWaitTimeout);
EXPECT_FALSE(called); EXPECT_FALSE(called.get());
} }
// Test that we can call AsyncInvoke with callback after the thread died. // Test that we can call AsyncInvoke with callback after the thread died.
@ -595,9 +627,9 @@ TEST_F(GuardedAsyncInvokeTest, KillThreadWithCallback) {
TEST_F(GuardedAsyncInvokeTest, FireAndForget) { TEST_F(GuardedAsyncInvokeTest, FireAndForget) {
GuardedAsyncInvoker invoker; GuardedAsyncInvoker invoker;
// Try calling functor. // Try calling functor.
bool called = false; AtomicBool called;
EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&called))); EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&called)));
EXPECT_TRUE_WAIT(called, kWaitTimeout); EXPECT_TRUE_WAIT(called.get(), kWaitTimeout);
} }
TEST_F(GuardedAsyncInvokeTest, WithCallback) { TEST_F(GuardedAsyncInvokeTest, WithCallback) {
@ -660,39 +692,39 @@ TEST_F(GuardedAsyncInvokeTest, KillInvokerBeforeExecute) {
TEST_F(GuardedAsyncInvokeTest, Flush) { TEST_F(GuardedAsyncInvokeTest, Flush) {
GuardedAsyncInvoker invoker; GuardedAsyncInvoker invoker;
bool flag1 = false; AtomicBool flag1;
bool flag2 = false; AtomicBool flag2;
// Queue two async calls to the current thread. // Queue two async calls to the current thread.
EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1))); EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1)));
EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2))); EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2)));
// Because we haven't pumped messages, these should not have run yet. // Because we haven't pumped messages, these should not have run yet.
EXPECT_FALSE(flag1); EXPECT_FALSE(flag1.get());
EXPECT_FALSE(flag2); EXPECT_FALSE(flag2.get());
// Force them to run now. // Force them to run now.
EXPECT_TRUE(invoker.Flush()); EXPECT_TRUE(invoker.Flush());
EXPECT_TRUE(flag1); EXPECT_TRUE(flag1.get());
EXPECT_TRUE(flag2); EXPECT_TRUE(flag2.get());
} }
TEST_F(GuardedAsyncInvokeTest, FlushWithIds) { TEST_F(GuardedAsyncInvokeTest, FlushWithIds) {
GuardedAsyncInvoker invoker; GuardedAsyncInvoker invoker;
bool flag1 = false; AtomicBool flag1;
bool flag2 = false; AtomicBool flag2;
// Queue two async calls to the current thread, one with a message id. // Queue two async calls to the current thread, one with a message id.
EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1), 5)); EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1), 5));
EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2))); EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2)));
// Because we haven't pumped messages, these should not have run yet. // Because we haven't pumped messages, these should not have run yet.
EXPECT_FALSE(flag1); EXPECT_FALSE(flag1.get());
EXPECT_FALSE(flag2); EXPECT_FALSE(flag2.get());
// Execute pending calls with id == 5. // Execute pending calls with id == 5.
EXPECT_TRUE(invoker.Flush(5)); EXPECT_TRUE(invoker.Flush(5));
EXPECT_TRUE(flag1); EXPECT_TRUE(flag1.get());
EXPECT_FALSE(flag2); EXPECT_FALSE(flag2.get());
flag1 = false; flag1 = false;
// Execute all pending calls. The id == 5 call should not execute again. // Execute all pending calls. The id == 5 call should not execute again.
EXPECT_TRUE(invoker.Flush()); EXPECT_TRUE(invoker.Flush());
EXPECT_FALSE(flag1); EXPECT_FALSE(flag1.get());
EXPECT_TRUE(flag2); EXPECT_TRUE(flag2.get());
} }
#if defined(WEBRTC_WIN) #if defined(WEBRTC_WIN)