From 35da3b67357ee73d255ce49073f6765b77519e61 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 20 May 2016 13:05:06 -0700 Subject: [PATCH] Fix race in worker_thread_test.cc The desired work count must be set before the thread is started, otherwise multiple work items might be completed before it is set, resulting it never signalling the sema. R=mark@chromium.org,rsesek@chromium.org BUG=crashpad:115 Change-Id: Ie4712f56af073277366cb84cca6d302a9eab409a Reviewed-on: https://chromium-review.googlesource.com/346193 Reviewed-by: Mark Mentovai --- util/thread/worker_thread_test.cc | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/util/thread/worker_thread_test.cc b/util/thread/worker_thread_test.cc index 24f0cb3d..0cbee187 100644 --- a/util/thread/worker_thread_test.cc +++ b/util/thread/worker_thread_test.cc @@ -34,10 +34,13 @@ class WorkDelegate : public WorkerThread::Delegate { semaphore_.Signal(); } - //! \brief Suspends the calling thread until the DoWork() has been called - //! the specified number of times. - void WaitForWorkCount(int times) { + void SetDesiredWorkCount(int times) { waiting_for_count_ = times; + } + + //! \brief Suspends the calling thread until the DoWork() has been called + //! the number of times specified by SetDesiredWorkCount(). + void WaitForWorkCount() { semaphore_.Wait(); } @@ -56,10 +59,11 @@ TEST(WorkerThread, DoWork) { WorkerThread thread(0.05, &delegate); uint64_t start = ClockMonotonicNanoseconds(); + delegate.SetDesiredWorkCount(2); thread.Start(0); EXPECT_TRUE(thread.is_running()); - delegate.WaitForWorkCount(2); + delegate.WaitForWorkCount(); thread.Stop(); EXPECT_FALSE(thread.is_running()); @@ -80,15 +84,17 @@ TEST(WorkerThread, Restart) { WorkDelegate delegate; WorkerThread thread(0.05, &delegate); + delegate.SetDesiredWorkCount(1); thread.Start(0); EXPECT_TRUE(thread.is_running()); - delegate.WaitForWorkCount(1); + delegate.WaitForWorkCount(); thread.Stop(); ASSERT_FALSE(thread.is_running()); + delegate.SetDesiredWorkCount(2); thread.Start(0); - delegate.WaitForWorkCount(2); + delegate.WaitForWorkCount(); thread.Stop(); ASSERT_FALSE(thread.is_running()); } @@ -97,16 +103,18 @@ TEST(WorkerThread, DoWorkNow) { WorkDelegate delegate; WorkerThread thread(100, &delegate); + delegate.SetDesiredWorkCount(1); thread.Start(0); EXPECT_TRUE(thread.is_running()); uint64_t start = ClockMonotonicNanoseconds(); - delegate.WaitForWorkCount(1); + delegate.WaitForWorkCount(); EXPECT_EQ(1, delegate.work_count()); + delegate.SetDesiredWorkCount(2); thread.DoWorkNow(); - delegate.WaitForWorkCount(2); + delegate.WaitForWorkCount(); thread.Stop(); EXPECT_EQ(2, delegate.work_count()); @@ -119,11 +127,12 @@ TEST(WorkerThread, DoWorkNowAtStart) { uint64_t start = ClockMonotonicNanoseconds(); + delegate.SetDesiredWorkCount(1); thread.Start(100); EXPECT_TRUE(thread.is_running()); thread.DoWorkNow(); - delegate.WaitForWorkCount(1); + delegate.WaitForWorkCount(); EXPECT_EQ(1, delegate.work_count()); EXPECT_GE(100 * kNanosecondsPerSecond, ClockMonotonicNanoseconds() - start);