diff options
author | Steve Fung <stevefung@google.com> | 2016-08-03 16:31:19 -0700 |
---|---|---|
committer | Steve Fung <stevefung@google.com> | 2016-08-03 16:31:25 -0700 |
commit | 5c7518c7bd1205dab9ba7701c95106f6a6195f3b (patch) | |
tree | 489d6a01b62ff33949bd95a2fbd0a8148070cb72 | |
parent | 7de8dfb2b917082486b21b1666c4687ec05e310b (diff) | |
download | libchrome-5c7518c7bd1205dab9ba7701c95106f6a6195f3b.tar.gz |
Fix a race condition in persistent memory allocator unit test
The PersistentMemoryAllocatorTest.IteratorParallelismTest unit
test gets stuck on a thread's Join() call when run on
brilloemulator-arm. This appears to be due to a race condition
in the thread setting up its lock and waiting on the condition
variable, and the parent thread issuing the Signal() chain that
is meant to wake up the children threads. Add in an autolock in
the main thread and a conditional bool around the threads' call
to Wait() to prevent any threads from calling Wait() after the
main thread has already sent the signal.
BUG=b:30183753
TEST=`base_unittests --gtest_filter=PersistentMemoryAllocatorTest.*`
passes
Committed: https://crrev.com/ff268f9b1002cedd94e6f6b9b3c6960aeb3037ca
Cr-Commit-Position: refs/heads/master@{#409622}
Change-Id: I6c5cf36f77655a5b34bce1b005ceda54101febef
-rw-r--r-- | base/metrics/persistent_memory_allocator_unittest.cc | 41 |
1 files changed, 32 insertions, 9 deletions
diff --git a/base/metrics/persistent_memory_allocator_unittest.cc b/base/metrics/persistent_memory_allocator_unittest.cc index 64333389a1..a3d90c2612 100644 --- a/base/metrics/persistent_memory_allocator_unittest.cc +++ b/base/metrics/persistent_memory_allocator_unittest.cc @@ -320,12 +320,14 @@ class CounterThread : public SimpleThread { CounterThread(const std::string& name, PersistentMemoryAllocator::Iterator* iterator, Lock* lock, - ConditionVariable* condition) + ConditionVariable* condition, + bool* wake_up) : SimpleThread(name, Options()), iterator_(iterator), lock_(lock), condition_(condition), - count_(0) {} + count_(0), + wake_up_(wake_up) {} void Run() override { // Wait so all threads can start at approximately the same time. @@ -333,8 +335,17 @@ class CounterThread : public SimpleThread { // releases the next, etc., etc. { AutoLock autolock(*lock_); - condition_->Wait(); - condition_->Signal(); + + // Before calling Wait(), make sure that the wake up condition + // has not already passed. Also, since spurious signal events + // are possible, check the condition in a while loop to make + // sure that the wake up condition is met when this thread + // returns from the Wait(). + // See usage comments in src/base/synchronization/condition_variable.h. + while (!*wake_up_) { + condition_->Wait(); + condition_->Signal(); + } } uint32_t type; @@ -350,6 +361,9 @@ class CounterThread : public SimpleThread { Lock* lock_; ConditionVariable* condition_; unsigned count_; + bool* wake_up_; + + DISALLOW_COPY_AND_ASSIGN(CounterThread); }; // Ensure that parallel iteration returns the same number of objects as @@ -373,12 +387,13 @@ TEST_F(PersistentMemoryAllocatorTest, IteratorParallelismTest) { PersistentMemoryAllocator::Iterator iter(allocator_.get()); Lock lock; ConditionVariable condition(&lock); + bool wake_up = false; - CounterThread t1("t1", &iter, &lock, &condition); - CounterThread t2("t2", &iter, &lock, &condition); - CounterThread t3("t3", &iter, &lock, &condition); - CounterThread t4("t4", &iter, &lock, &condition); - CounterThread t5("t5", &iter, &lock, &condition); + CounterThread t1("t1", &iter, &lock, &condition, &wake_up); + CounterThread t2("t2", &iter, &lock, &condition, &wake_up); + CounterThread t3("t3", &iter, &lock, &condition, &wake_up); + CounterThread t4("t4", &iter, &lock, &condition, &wake_up); + CounterThread t5("t5", &iter, &lock, &condition, &wake_up); t1.Start(); t2.Start(); @@ -386,6 +401,14 @@ TEST_F(PersistentMemoryAllocatorTest, IteratorParallelismTest) { t4.Start(); t5.Start(); + // Take the lock and set the wake up condition to true. This helps to + // avoid a race condition where the Signal() event is called before + // all the threads have reached the Wait() and thus never get woken up. + { + AutoLock autolock(lock); + wake_up = true; + } + // This will release all the waiting threads. condition.Signal(); |