From efc5ae94f96606d4f876928b2b56ac91d794f9c5 Mon Sep 17 00:00:00 2001 From: skvlad Date: Wed, 5 Oct 2016 15:58:12 -0700 Subject: [PATCH] Fixed flaky SharedExclusiveLock tests. These tests were checking the behavior of thread synchronization primitives using nothing but carefully timed sleeps. This was very unreliable and prone to flakes. This CL replaces the sleeps with waiting on synchronization events. There is still the need to wait for timeouts when testing for negative outcomes, but it's greatly reduced. I've run these tests for thousands of iterations on MSan without a single failure. BUG=webrtc:5824 R=pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/2393023002 . Cr-Commit-Position: refs/heads/master@{#14534} --- webrtc/base/sharedexclusivelock_unittest.cc | 84 ++++++--------------- 1 file changed, 25 insertions(+), 59 deletions(-) diff --git a/webrtc/base/sharedexclusivelock_unittest.cc b/webrtc/base/sharedexclusivelock_unittest.cc index 5fbf320c82..3886fb27ab 100644 --- a/webrtc/base/sharedexclusivelock_unittest.cc +++ b/webrtc/base/sharedexclusivelock_unittest.cc @@ -12,22 +12,13 @@ #include "webrtc/base/common.h" #include "webrtc/base/gunit.h" +#include "webrtc/base/event.h" #include "webrtc/base/messagehandler.h" #include "webrtc/base/messagequeue.h" #include "webrtc/base/sharedexclusivelock.h" #include "webrtc/base/thread.h" #include "webrtc/base/timeutils.h" -#if defined(MEMORY_SANITIZER) -// Flaky under MemorySanitizer, see -// https://bugs.chromium.org/p/webrtc/issues/detail?id=5824 -#define MAYBE_TestSharedExclusive DISABLED_TestSharedExclusive -#define MAYBE_TestExclusiveExclusive DISABLED_TestExclusiveExclusive -#else -#define MAYBE_TestSharedExclusive TestSharedExclusive -#define MAYBE_TestExclusiveExclusive TestExclusiveExclusive -#endif - namespace rtc { static const uint32_t kMsgRead = 0; @@ -41,28 +32,24 @@ class SharedExclusiveTask : public MessageHandler { public: SharedExclusiveTask(SharedExclusiveLock* shared_exclusive_lock, int* value, - bool* done) + Event* done) : shared_exclusive_lock_(shared_exclusive_lock), - waiting_time_in_ms_(0), value_(value), done_(done) { worker_thread_.reset(new Thread()); worker_thread_->Start(); } - int64_t waiting_time_in_ms() const { return waiting_time_in_ms_; } - protected: std::unique_ptr worker_thread_; SharedExclusiveLock* shared_exclusive_lock_; - int64_t waiting_time_in_ms_; int* value_; - bool* done_; + Event* done_; }; class ReadTask : public SharedExclusiveTask { public: - ReadTask(SharedExclusiveLock* shared_exclusive_lock, int* value, bool* done) + ReadTask(SharedExclusiveLock* shared_exclusive_lock, int* value, Event* done) : SharedExclusiveTask(shared_exclusive_lock, value, done) { } @@ -80,14 +67,10 @@ class ReadTask : public SharedExclusiveTask { TypedMessageData* message_data = static_cast*>(message->pdata); - int64_t start_time = TimeMillis(); { SharedScope ss(shared_exclusive_lock_); - waiting_time_in_ms_ = TimeDiff(TimeMillis(), start_time); - - Thread::SleepMs(kProcessTimeInMs); *message_data->data() = *value_; - *done_ = true; + done_->Set(); } delete message->pdata; message->pdata = NULL; @@ -96,7 +79,7 @@ class ReadTask : public SharedExclusiveTask { class WriteTask : public SharedExclusiveTask { public: - WriteTask(SharedExclusiveLock* shared_exclusive_lock, int* value, bool* done) + WriteTask(SharedExclusiveLock* shared_exclusive_lock, int* value, Event* done) : SharedExclusiveTask(shared_exclusive_lock, value, done) { } @@ -114,14 +97,10 @@ class WriteTask : public SharedExclusiveTask { TypedMessageData* message_data = static_cast*>(message->pdata); - int64_t start_time = TimeMillis(); { ExclusiveScope es(shared_exclusive_lock_); - waiting_time_in_ms_ = TimeDiff(TimeMillis(), start_time); - - Thread::SleepMs(kProcessTimeInMs); *value_ = message_data->data(); - *done_ = true; + done_->Set(); } delete message->pdata; message->pdata = NULL; @@ -144,10 +123,9 @@ class SharedExclusiveLockTest int value_; }; -// Flaky: https://code.google.com/p/webrtc/issues/detail?id=3318 TEST_F(SharedExclusiveLockTest, TestSharedShared) { int value0, value1; - bool done0, done1; + Event done0(false, false), done1(false, false); ReadTask reader0(shared_exclusive_lock_.get(), &value_, &done0); ReadTask reader1(shared_exclusive_lock_.get(), &value_, &done1); @@ -155,77 +133,65 @@ TEST_F(SharedExclusiveLockTest, TestSharedShared) { { SharedScope ss(shared_exclusive_lock_.get()); value_ = 1; - done0 = false; - done1 = false; reader0.PostRead(&value0); reader1.PostRead(&value1); - Thread::SleepMs(kProcessTimeInMs); - } - EXPECT_TRUE_WAIT(done0, kProcessTimeoutInMs); - EXPECT_EQ(1, value0); - EXPECT_LE(reader0.waiting_time_in_ms(), kNoWaitThresholdInMs); - EXPECT_TRUE_WAIT(done1, kProcessTimeoutInMs); - EXPECT_EQ(1, value1); - EXPECT_LE(reader1.waiting_time_in_ms(), kNoWaitThresholdInMs); + EXPECT_TRUE(done0.Wait(kProcessTimeoutInMs)); + EXPECT_TRUE(done1.Wait(kProcessTimeoutInMs)); + EXPECT_EQ(1, value0); + EXPECT_EQ(1, value1); + } } -TEST_F(SharedExclusiveLockTest, MAYBE_TestSharedExclusive) { - bool done; +TEST_F(SharedExclusiveLockTest, TestSharedExclusive) { + Event done(false, false); WriteTask writer(shared_exclusive_lock_.get(), &value_, &done); // Test exclusive lock needs to wait for shared lock. { SharedScope ss(shared_exclusive_lock_.get()); value_ = 1; - done = false; writer.PostWrite(2); - Thread::SleepMs(kProcessTimeInMs); - EXPECT_EQ(1, value_); + EXPECT_FALSE(done.Wait(kProcessTimeInMs)); } - - EXPECT_TRUE_WAIT(done, kProcessTimeoutInMs); + EXPECT_TRUE(done.Wait(kProcessTimeoutInMs)); EXPECT_EQ(2, value_); - EXPECT_GE(writer.waiting_time_in_ms(), kWaitThresholdInMs); } TEST_F(SharedExclusiveLockTest, TestExclusiveShared) { int value; - bool done; + Event done(false, false); ReadTask reader(shared_exclusive_lock_.get(), &value_, &done); // Test shared lock needs to wait for exclusive lock. { ExclusiveScope es(shared_exclusive_lock_.get()); value_ = 1; - done = false; reader.PostRead(&value); - Thread::SleepMs(kProcessTimeInMs); + EXPECT_FALSE(done.Wait(kProcessTimeInMs)); value_ = 2; } - EXPECT_TRUE_WAIT(done, kProcessTimeoutInMs); + EXPECT_TRUE(done.Wait(kProcessTimeoutInMs)); EXPECT_EQ(2, value); - EXPECT_GE(reader.waiting_time_in_ms(), kWaitThresholdInMs); } -TEST_F(SharedExclusiveLockTest, MAYBE_TestExclusiveExclusive) { - bool done; +TEST_F(SharedExclusiveLockTest, TestExclusiveExclusive) { + Event done(false, false); WriteTask writer(shared_exclusive_lock_.get(), &value_, &done); // Test exclusive lock needs to wait for exclusive lock. { ExclusiveScope es(shared_exclusive_lock_.get()); + // Start the writer task only after holding the lock, to ensure it need value_ = 1; - done = false; writer.PostWrite(2); - Thread::SleepMs(kProcessTimeInMs); + EXPECT_FALSE(done.Wait(kProcessTimeInMs)); EXPECT_EQ(1, value_); } - EXPECT_TRUE_WAIT(done, kProcessTimeoutInMs); + EXPECT_TRUE(done.Wait(kProcessTimeoutInMs)); EXPECT_EQ(2, value_); - EXPECT_GE(writer.waiting_time_in_ms(), kWaitThresholdInMs); } } // namespace rtc