diff options
| -rw-r--r-- | base/condition_variable_unittest.cc | 38 | ||||
| -rw-r--r-- | base/lock.h | 2 | ||||
| -rw-r--r-- | base/lock_impl.h | 4 | ||||
| -rw-r--r-- | base/lock_impl_win.cc | 2 |
4 files changed, 35 insertions, 11 deletions
diff --git a/base/condition_variable_unittest.cc b/base/condition_variable_unittest.cc index 8e47114..6bdb96b 100644 --- a/base/condition_variable_unittest.cc +++ b/base/condition_variable_unittest.cc @@ -77,12 +77,12 @@ class WorkQueue : public PlatformThread::Delegate { 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(); @@ -103,8 +103,14 @@ class WorkQueue : public PlatformThread::Delegate { 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_; @@ -176,6 +182,12 @@ TEST_F(ConditionVariableTest, TimeoutTest) { // 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. @@ -336,7 +348,7 @@ TEST_F(ConditionVariableTest, DISABLED_MultiThreadConsumerTest) { 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. } @@ -436,7 +448,7 @@ TEST_F(ConditionVariableTest, LargeFastTaskTest) { // 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. } @@ -519,16 +531,27 @@ bool WorkQueue::allow_help_requests() const { } 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_++; } @@ -606,6 +629,7 @@ void WorkQueue::SetAllowHelp(bool allow) { } void WorkQueue::SetShutdown() { + lock_.AssertAcquired(); shutdown_ = true; } diff --git a/base/lock.h b/base/lock.h index f3653cf..31ad9a0 100644 --- a/base/lock.h +++ b/base/lock.h @@ -23,7 +23,7 @@ class Lock { // calling thread. If the lock has not been acquired, then the method // will DCHECK(). In non-debug builds, the LockImpl's implementation of // AssertAcquired() is an empty inline method. - void AssertAcquired() { return lock_.AssertAcquired(); } + void AssertAcquired() const { return lock_.AssertAcquired(); } // Return the underlying lock implementation. // TODO(awalker): refactor lock and condition variables so that this is diff --git a/base/lock_impl.h b/base/lock_impl.h index d3fcc19..2d4a921 100644 --- a/base/lock_impl.h +++ b/base/lock_impl.h @@ -47,9 +47,9 @@ class LockImpl { // through the os_lock() method, runtime assertions can not be done on those // builds. #if defined(NDEBUG) || !defined(OS_WIN) - void AssertAcquired() {} + void AssertAcquired() const {} #else - void AssertAcquired(); + void AssertAcquired() const; #endif // Return the native underlying lock. Not supported for Windows builds. diff --git a/base/lock_impl_win.cc b/base/lock_impl_win.cc index 503f105..0f0e424 100644 --- a/base/lock_impl_win.cc +++ b/base/lock_impl_win.cc @@ -66,7 +66,7 @@ void LockImpl::Unlock() { // In non-debug builds, this method is declared as an empty inline method. #ifndef NDEBUG -void LockImpl::AssertAcquired() { +void LockImpl::AssertAcquired() const { DCHECK(recursion_count_shadow_ > 0); DCHECK_EQ(owning_thread_id_, PlatformThread::CurrentId()); } |