Modify WaitableEvent::Wait() to return void

Currently, WaitableEvent::Wait() returns bool. However, the Windows implementation DCHECKs that the return value is true and the POSIX implementation can never return false. Also, all call sites that use the return value simply DCHECK that it's true. This change modifies the method to return void, adds a DCHECK in the POSIX implementation and updates call sites. Review URL: http://codereview.chromium.org/8221021 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@104990 0039d316-1c4b-4281-b951-d872f2087c98
Denger-Network · Oct 12, 2011 · 866cf33 · 866cf33
1 parent 8d3fa5e
commit 866cf33
Show file tree

Hide file tree

Showing 15 changed files with 43 additions and 52 deletions.
diff --git a/base/synchronization/waitable_event.h b/base/synchronization/waitable_event.h
@@ -73,9 +73,8 @@ class BASE_EXPORT WaitableEvent {
   // is not a manual reset event, then this test will cause a reset.
   bool IsSignaled();
 
-  // Wait indefinitely for the event to be signaled.  Returns true if the event
-  // was signaled, else false is returned to indicate that waiting failed.
-  bool Wait();
+  // Wait indefinitely for the event to be signaled.
+  void Wait();
 
   // Wait up until max_time has passed for the event to be signaled.  Returns
   // true if the event was signaled.  If this method returns false, then it

diff --git a/base/synchronization/waitable_event_posix.cc b/base/synchronization/waitable_event_posix.cc
@@ -149,30 +149,31 @@ class SyncWaiter : public WaitableEvent::Waiter {
   base::ConditionVariable cv_;
 };
 
-bool WaitableEvent::Wait() {
-  return TimedWait(TimeDelta::FromSeconds(-1));
+void WaitableEvent::Wait() {
+  bool result = TimedWait(TimeDelta::FromSeconds(-1));
+  DCHECK(result) << "TimedWait() should never fail with infinite timeout";
 }
 
 bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
   const Time end_time(Time::Now() + max_time);
   const bool finite_time = max_time.ToInternalValue() >= 0;
 
   kernel_->lock_.Acquire();
-    if (kernel_->signaled_) {
-      if (!kernel_->manual_reset_) {
-        // In this case we were signaled when we had no waiters. Now that
-        // someone has waited upon us, we can automatically reset.
-        kernel_->signaled_ = false;
-      }
-
-      kernel_->lock_.Release();
-      return true;
+  if (kernel_->signaled_) {
+    if (!kernel_->manual_reset_) {
+      // In this case we were signaled when we had no waiters. Now that
+      // someone has waited upon us, we can automatically reset.
+      kernel_->signaled_ = false;
     }
 
-    SyncWaiter sw;
-    sw.lock()->Acquire();
+    kernel_->lock_.Release();
+    return true;
+  }
+
+  SyncWaiter sw;
+  sw.lock()->Acquire();
 
-    Enqueue(&sw);
+  Enqueue(&sw);
   kernel_->lock_.Release();
   // We are violating locking order here by holding the SyncWaiter lock but not
   // the WaitableEvent lock. However, this is safe because we don't lock @lock_
@@ -193,7 +194,7 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
       sw.lock()->Release();
 
       kernel_->lock_.Acquire();
-        kernel_->Dequeue(&sw, &sw);
+      kernel_->Dequeue(&sw, &sw);
       kernel_->lock_.Release();
 
       return return_value;

diff --git a/base/synchronization/waitable_event_unittest.cc b/base/synchronization/waitable_event_unittest.cc
@@ -23,7 +23,7 @@ TEST(WaitableEventTest, ManualBasics) {
   EXPECT_FALSE(event.TimedWait(TimeDelta::FromMilliseconds(10)));
 
   event.Signal();
-  EXPECT_TRUE(event.Wait());
+  event.Wait();
   EXPECT_TRUE(event.TimedWait(TimeDelta::FromMilliseconds(10)));
 }
 
@@ -41,7 +41,7 @@ TEST(WaitableEventTest, AutoBasics) {
   EXPECT_FALSE(event.TimedWait(TimeDelta::FromMilliseconds(10)));
 
   event.Signal();
-  EXPECT_TRUE(event.Wait());
+  event.Wait();
   EXPECT_FALSE(event.TimedWait(TimeDelta::FromMilliseconds(10)));
 
   event.Signal();

diff --git a/base/synchronization/waitable_event_win.cc b/base/synchronization/waitable_event_win.cc
@@ -46,12 +46,11 @@ bool WaitableEvent::IsSignaled() {
   return TimedWait(TimeDelta::FromMilliseconds(0));
 }
 
-bool WaitableEvent::Wait() {
+void WaitableEvent::Wait() {
   DWORD result = WaitForSingleObject(handle_, INFINITE);
   // It is most unexpected that this should ever fail.  Help consumers learn
   // about it if it should ever fail.
   DCHECK_EQ(WAIT_OBJECT_0, result) << "WaitForSingleObject failed";
-  return result == WAIT_OBJECT_0;
 }
 
 bool WaitableEvent::TimedWait(const TimeDelta& max_time) {

diff --git a/base/threading/worker_pool_posix_unittest.cc b/base/threading/worker_pool_posix_unittest.cc
@@ -106,7 +106,7 @@ class BlockingIncrementingTask : public Task {
       (*num_waiting_to_start_)++;
     }
     num_waiting_to_start_cv_->Signal();
-    CHECK(start_->Wait());
+    start_->Wait();
     incrementer_.Run();
   }
 

diff --git a/base/threading/worker_pool_unittest.cc b/base/threading/worker_pool_unittest.cc
@@ -34,15 +34,12 @@ class PostTaskTestTask : public Task {
 TEST_F(WorkerPoolTest, PostTask) {
   WaitableEvent test_event(false, false);
   WaitableEvent long_test_event(false, false);
-  bool signaled;
 
   WorkerPool::PostTask(FROM_HERE, new PostTaskTestTask(&test_event), false);
   WorkerPool::PostTask(FROM_HERE, new PostTaskTestTask(&long_test_event), true);
 
-  signaled = test_event.Wait();
-  EXPECT_TRUE(signaled);
-  signaled = long_test_event.Wait();
-  EXPECT_TRUE(signaled);
+  test_event.Wait();
+  long_test_event.Wait();
 }
 
 }  // namespace base
diff --git a/chrome/browser/extensions/test_extension_prefs.cc b/chrome/browser/extensions/test_extension_prefs.cc
@@ -80,7 +80,7 @@ void TestExtensionPrefs::RecreateExtensionPrefs() {
     // (otherwise the Wait below will hang).
     MessageLoop::current()->RunAllPending();
 
-    EXPECT_TRUE(io_finished.Wait());
+    io_finished.Wait();
   }
 
   extension_pref_value_map_.reset(new ExtensionPrefValueMap);

diff --git a/chrome/browser/net/cookie_policy_browsertest.cc b/chrome/browser/net/cookie_policy_browsertest.cc
@@ -68,7 +68,7 @@ class CookiePolicyBrowserTest : public InProcessBrowserTest {
         BrowserThread::PostTask(
             BrowserThread::IO, FROM_HERE,
             new GetCookiesTask(url, context_getter, &event, &cookies)));
-    EXPECT_TRUE(event.Wait());
+    event.Wait();
     return cookies;
   }
 

diff --git a/chrome/browser/plugin_data_remover.cc b/chrome/browser/plugin_data_remover.cc
@@ -71,14 +71,12 @@ base::WaitableEvent* PluginDataRemover::StartRemoving(base::Time begin_time) {
 
 void PluginDataRemover::Wait() {
   base::Time start_time(base::Time::Now());
-  bool result = true;
   if (is_removing_)
-    result = event_->Wait();
+    event_->Wait();
   UMA_HISTOGRAM_TIMES("ClearPluginData.wait_at_shutdown",
                       base::Time::Now() - start_time);
   UMA_HISTOGRAM_TIMES("ClearPluginData.time_at_shutdown",
                       base::Time::Now() - remove_start_time_);
-  DCHECK(result) << "Error waiting for plugin process";
 }
 
 int PluginDataRemover::ID() {

diff --git a/chrome/browser/process_singleton_uitest.cc b/chrome/browser/process_singleton_uitest.cc
@@ -92,7 +92,7 @@ class ChromeStarter : public base::RefCountedThreadSafe<ChromeStarter> {
     ready_event_.Signal();
     // And then wait for the test to tell us to GO!
     ASSERT_NE(static_cast<base::WaitableEvent*>(NULL), start_event);
-    ASSERT_TRUE(start_event->Wait());
+    start_event->Wait();
 
     // Here we don't wait for the app to be terminated because one of the
     // process will stay alive while the others will be restarted. If we would
@@ -266,7 +266,7 @@ TEST_F(ProcessSingletonTest, MAYBE_StartupRaceCondition) {
     // We could replace this loop if we ever implement a WaitAll().
     for (size_t i = 0; i < kNbThreads; ++i) {
       SCOPED_TRACE(testing::Message() << "Waiting on thread: " << i << ".");
-      ASSERT_TRUE(chrome_starters_[i]->ready_event_.Wait());
+      chrome_starters_[i]->ready_event_.Wait();
     }
     // GO!
     threads_waker_.Signal();

diff --git a/chrome/browser/sync/glue/http_bridge.cc b/chrome/browser/sync/glue/http_bridge.cc
@@ -198,9 +198,9 @@ bool HttpBridge::MakeSynchronousPost(int* error_code, int* response_code) {
     return false;
   }
 
-  if (!http_post_completed_.Wait())  // Block until network request completes
-    NOTREACHED();                    // or is aborted. See OnURLFetchComplete
-                                     // and Abort.
+  // Block until network request completes or is aborted. See
+  // OnURLFetchComplete and Abort.
+  http_post_completed_.Wait();
 
   base::AutoLock lock(fetch_state_lock_);
   DCHECK(fetch_state_.request_completed || fetch_state_.aborted);

diff --git a/chrome/service/gaia/service_gaia_authenticator.cc b/chrome/service/gaia/service_gaia_authenticator.cc
@@ -34,8 +34,8 @@ bool ServiceGaiaAuthenticator::Post(const GURL& url,
                         post_body));
   // TODO(sanjeevr): Waiting here until the network request completes is not
   // desirable. We need to change Post to be asynchronous.
-  if (!http_post_completed_.Wait())  // Block until network request completes.
-    NOTREACHED();                    // See OnURLFetchComplete.
+  // Block until network request completes. See OnURLFetchComplete.
+  http_post_completed_.Wait();
 
   *response_code = static_cast<int>(http_response_code_);
   *response_body = response_data_;

diff --git a/content/browser/in_process_webkit/indexed_db_key_utility_client.cc b/content/browser/in_process_webkit/indexed_db_key_utility_client.cc
@@ -197,9 +197,9 @@ void KeyUtilityClientImpl::StartUtilityProcess() {
   DCHECK(state_ == STATE_UNINITIALIZED);
 
   GetRDHAndStartUtilityProcess();
-  bool ret = waitable_event_.Wait();
+  waitable_event_.Wait();
 
-  DCHECK(ret && state_ == STATE_INITIALIZED);
+  DCHECK(state_ == STATE_INITIALIZED);
 }
 
 void KeyUtilityClientImpl::CreateIDBKeysFromSerializedValuesAndKeyPath(
@@ -216,8 +216,8 @@ void KeyUtilityClientImpl::CreateIDBKeysFromSerializedValuesAndKeyPath(
 
   state_ = STATE_CREATING_KEYS;
   CallStartIDBKeyFromValueAndKeyPathFromIOThread(values, key_path);
-  bool ret = waitable_event_.Wait();
-  DCHECK(ret && state_ == STATE_INITIALIZED);
+  waitable_event_.Wait();
+  DCHECK(state_ == STATE_INITIALIZED);
 
   *keys = keys_;
 }
@@ -235,8 +235,8 @@ SerializedScriptValue KeyUtilityClientImpl::InjectIDBKeyIntoSerializedValue(
   state_ = STATE_INJECTING_KEY;
   CallStartInjectIDBKeyFromIOThread(key, value, key_path);
 
-  bool ret = waitable_event_.Wait();
-  DCHECK(ret && state_ == STATE_INITIALIZED);
+  waitable_event_.Wait();
+  DCHECK(state_ == STATE_INITIALIZED);
 
   return value_after_injection_;
 }

diff --git a/net/base/keygen_handler_unittest.cc b/net/base/keygen_handler_unittest.cc
@@ -133,8 +133,7 @@ TEST_F(KeygenHandlerTest, ConcurrencyTest) {
 
   for (int i = 0; i < NUM_HANDLERS; i++) {
     // Make sure the job completed
-    bool signaled = events[i]->Wait();
-    EXPECT_TRUE(signaled);
+    events[i]->Wait();
     delete events[i];
     events[i] = NULL;
 

diff --git a/webkit/tools/test_shell/simple_resource_loader_bridge.cc b/webkit/tools/test_shell/simple_resource_loader_bridge.cc
@@ -698,8 +698,7 @@ class SyncRequestProxy : public RequestProxy {
   }
 
   void WaitForCompletion() {
-    if (!event_.Wait())
-      NOTREACHED();
+    event_.Wait();
   }
 
   // --------------------------------------------------------------------------
@@ -889,8 +888,7 @@ class CookieGetter : public base::RefCountedThreadSafe<CookieGetter> {
   }
 
   std::string GetResult() {
-    if (!event_.Wait())
-      NOTREACHED();
+    event_.Wait();
     return result_;
   }