Test was examining a variable without first acquiring a required Lock....

base/condition_variable_unittest.cc

 // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Multi-threaded tests of ConditionVariable class.
 
 #include <time.h>
 #include <algorithm>
 #include <vector>
 
@@ skipping 59 matching lines @@
   // Worker threads only call the following methods.
   // They should use the lock to get exclusive access.
   int GetThreadId();  // Get an ID assigned to a thread..
   bool EveryIdWasAllocated() const;  // Indicates that all IDs were handed out.
   TimeDelta GetAnAssignment(int thread_id);  // Get a work task duration.
   void WorkIsCompleted(int thread_id);
 
   int task_count() const;
   bool allow_help_requests() const;  // Workers can signal more workers.
   bool shutdown() const;  // Check if shutdown has been requested.
-  int shutdown_task_count() const;
 
   void thread_shutting_down();
 
+
   //----------------------------------------------------------------------------
-  // Worker threads can call them but not needed to acquire a lock
+  // Worker threads can call them but not needed to acquire a lock.
   Lock* lock();
 
   ConditionVariable* work_is_available();
   ConditionVariable* all_threads_have_ids();
   ConditionVariable* no_more_tasks();
 
   //----------------------------------------------------------------------------
   // The rest of the methods are for use by the controlling master thread (the
   // test case code).
   void ResetHistory();
   int GetMinCompletionsByWorkerThread() const;
   int GetMaxCompletionsByWorkerThread() const;
   int GetNumThreadsTakingAssignments() const;
   int GetNumThreadsCompletingTasks() const;
   int GetNumberOfCompletedTasks() const;
 
   void SetWorkTime(TimeDelta delay);
   void SetTaskCount(int count);
   void SetAllowHelp(bool allow);
 
+  // Caller must acquire lock before calling.
   void SetShutdown();
 
+  // Compares the |shutdown_task_count_| to the |thread_count| and returns true
+  // if they are equal.  This check will acquire the |lock_| so the caller
+  // should not hold the lock when calling this method.
+  bool ThreadSafeCheckShutdown(int thread_count);
+
  private:
   // Both worker threads and controller use the following to synchronize.
   Lock lock_;
   ConditionVariable work_is_available_;  // To tell threads there is work.
 
   // Conditions to notify the controlling process (if it is interested).
   ConditionVariable all_threads_have_ids_;  // All threads are running.
   ConditionVariable no_more_tasks_;  // Task count is zero.
 
   const int thread_count_;
@@ skipping 51 matching lines @@
   TimeDelta duration = TimeTicks::Now() - start;
   // We can't use EXPECT_GE here as the TimeDelta class does not support the
   // required stream conversion.
   EXPECT_TRUE(duration >= WAIT_TIME);
 
   lock.Release();
 }
 
 // This test is flaky due to excessive timing sensitivity.
 // http://code.google.com/p/chromium/issues/detail?id=3599
+// TODO(jar): A recent change to the WorkQueue fixed flakyness for the test
+// LargeFastTaskTest.  Specifically, the test was accessing the member variable
+// WorkQueue::shutdown_task_count_ without a lock held.  The MultiThreadConsumer
+// test now ran successfully for 500 times on RalphL's machine.  RalphL did not
+// want to blindly re-enable this test if you know of other issues with it, but
+// it appears that the flakyness is gone, but I'll leave that to you to verify.
 TEST_F(ConditionVariableTest, DISABLED_MultiThreadConsumerTest) {
   const int kThreadCount = 10;
   WorkQueue queue(kThreadCount);  // Start the threads.
 
   Lock private_lock;  // Used locally for master to wait.
   AutoLock private_held_lock(private_lock);
   ConditionVariable private_cv(&private_lock);
 
   {
     AutoLock auto_lock(*queue.lock());
@@ skipping 140 matching lines @@
     EXPECT_EQ(0, queue.task_count());
     EXPECT_EQ(2, queue.GetMaxCompletionsByWorkerThread());
     EXPECT_EQ(2, queue.GetMinCompletionsByWorkerThread());
     EXPECT_EQ(20, queue.GetNumberOfCompletedTasks());
 
     queue.SetShutdown();
   }
   queue.work_is_available()->Broadcast();  // Force check for shutdown.
 
   SPIN_FOR_TIMEDELTA_OR_UNTIL_TRUE(TimeDelta::FromMinutes(1),
-                                   queue.shutdown_task_count() == kThreadCount);
+                                   queue.ThreadSafeCheckShutdown(kThreadCount));
   PlatformThread::Sleep(10);  // Be sure they're all shutdown.
 }
 
 TEST_F(ConditionVariableTest, LargeFastTaskTest) {
   const int kThreadCount = 200;
   WorkQueue queue(kThreadCount);  // Start the threads.
 
   Lock private_lock;  // Used locally for master to wait.
   AutoLock private_held_lock(private_lock);
   ConditionVariable private_cv(&private_lock);
@@ skipping 79 matching lines @@
     EXPECT_EQ(0, queue.task_count());
     EXPECT_LE(4, queue.GetMaxCompletionsByWorkerThread());
     EXPECT_EQ(4 * kThreadCount, queue.GetNumberOfCompletedTasks());
 
     queue.SetShutdown();
   }
   queue.work_is_available()->Broadcast();  // Force check for shutdown.
 
   // Wait for shutdowns to complete.
   SPIN_FOR_TIMEDELTA_OR_UNTIL_TRUE(TimeDelta::FromMinutes(1),
-                                   queue.shutdown_task_count() == kThreadCount);
+                                   queue.ThreadSafeCheckShutdown(kThreadCount));
   PlatformThread::Sleep(10);  // Be sure they're all shutdown.
 }
 
 //------------------------------------------------------------------------------
 // Finally we provide the implementation for the methods in the WorkQueue class.
 //------------------------------------------------------------------------------
 
 WorkQueue::WorkQueue(int thread_count)
   : lock_(),
     work_is_available_(&lock_),
@@ skipping 62 matching lines @@
   DFAKE_SCOPED_RECURSIVE_LOCK(locked_methods_);
   return task_count_;
 }
 
 bool WorkQueue::allow_help_requests() const {
   DFAKE_SCOPED_RECURSIVE_LOCK(locked_methods_);
   return allow_help_requests_;
 }
 
 bool WorkQueue::shutdown() const {
+  lock_.AssertAcquired();
   DFAKE_SCOPED_RECURSIVE_LOCK(locked_methods_);
   return shutdown_;
 }
 
-int WorkQueue::shutdown_task_count() const {
-  DFAKE_SCOPED_RECURSIVE_LOCK(locked_methods_);
-  return shutdown_task_count_;
+// Because this method is called from the test's main thread we need to actually
+// take the lock.  Threads will call the thread_shutting_down() method with the
+// lock already acquired.
+bool WorkQueue::ThreadSafeCheckShutdown(int thread_count) {
+  bool all_shutdown;
+  AutoLock auto_lock(lock_);
+  {
+    // Declare in scope so DFAKE is guranteed to be destroyed before AutoLock.
+    DFAKE_SCOPED_RECURSIVE_LOCK(locked_methods_);
+    all_shutdown = (shutdown_task_count_ == thread_count);
+  }
+  return all_shutdown;
 }
 
 void WorkQueue::thread_shutting_down() {
+  lock_.AssertAcquired();
   DFAKE_SCOPED_RECURSIVE_LOCK(locked_methods_);
   shutdown_task_count_++;
 }
 
 Lock* WorkQueue::lock() {
   return &lock_;
 }
 
 ConditionVariable* WorkQueue::work_is_available() {
   return &work_is_available_;
@@ skipping 57 matching lines @@
 
 void WorkQueue::SetTaskCount(int count) {
   task_count_ = count;
 }
 
 void WorkQueue::SetAllowHelp(bool allow) {
   allow_help_requests_ = allow;
 }
 
 void WorkQueue::SetShutdown() {
+  lock_.AssertAcquired();
   shutdown_ = true;
 }
 
 //------------------------------------------------------------------------------
 // Define the standard worker task. Several tests will spin out many of these
 // threads.
 //------------------------------------------------------------------------------
 
 // The multithread tests involve several threads with a task to perform as
 // directed by an instance of the class WorkQueue.
@@ skipping 50 matching lines @@
 
     {
       AutoLock auto_lock(lock_);
       // Send notification that we completed our "work."
       WorkIsCompleted(thread_id);
     }
   }
 }
 
 }  // namespace