Modify EventPosix to prevent spurious wakeups.

pthread_cond_{timedwait,wait} are allowed to spuriously wake up as if
they were signaled. To prevent this being interpreted as a "real"
signaling of the event (ThreadWrapper for instance depends on it being
an actual signal) we need to check whether the event was actually
signalled or not.

BUG=4413
R=andresp@webrtc.org, tommi@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/49369004

Cr-Commit-Position: refs/heads/master@{#8752}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8752 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/system_wrappers/source/event_posix.cc

@@ -26,64 +26,30 @@ const long int E6 = 1000000;
 const long int E9 = 1000 * E6;
 
 EventWrapper* EventPosix::Create() {
-  EventPosix* ptr = new EventPosix;
+  return new EventPosix();
-  if (!ptr) {
-    return NULL;
-  }
-
-  const int error = ptr->Construct();
-  if (error) {
-    delete ptr;
-    return NULL;
-  }
-  return ptr;
 }
 
 EventPosix::EventPosix()
-    : timer_thread_(0),
+    : event_set_(false),
+      timer_thread_(nullptr),
       timer_event_(0),
+      created_at_(),
       periodic_(false),
       time_(0),
-      count_(0),
+      count_(0) {
-      state_(kDown) {
-}
-
-int EventPosix::Construct() {
-  // Set start time to zero
-  memset(&created_at_, 0, sizeof(created_at_));
-
   pthread_mutexattr_t attr;
   pthread_mutexattr_init(&attr);
   pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
-  int result = pthread_mutex_init(&mutex_, &attr);
+  pthread_mutex_init(&mutex_, &attr);
-  if (result != 0) {
-    return -1;
-  }
 #ifdef WEBRTC_CLOCK_TYPE_REALTIME
-  result = pthread_cond_init(&cond_, 0);
+  pthread_cond_init(&cond_, 0);
-  if (result != 0) {
-    return -1;
-  }
 #else
   pthread_condattr_t cond_attr;
-  result = pthread_condattr_init(&cond_attr);
+  pthread_condattr_init(&cond_attr);
-  if (result != 0) {
+  pthread_condattr_setclock(&cond_attr, CLOCK_MONOTONIC);
-    return -1;
+  pthread_cond_init(&cond_, &cond_attr);
-  }
+  pthread_condattr_destroy(&cond_attr);
-  result = pthread_condattr_setclock(&cond_attr, CLOCK_MONOTONIC);
-  if (result != 0) {
-    return -1;
-  }
-  result = pthread_cond_init(&cond_, &cond_attr);
-  if (result != 0) {
-    return -1;
-  }
-  result = pthread_condattr_destroy(&cond_attr);
-  if (result != 0) {
-    return -1;
-  }
 #endif
-  return 0;
 }
 
 EventPosix::~EventPosix() {
@@ -92,24 +58,20 @@ EventPosix::~EventPosix() {
   pthread_mutex_destroy(&mutex_);
 }
 
+// TODO(pbos): Make this void.
 bool EventPosix::Set() {
-  if (0 != pthread_mutex_lock(&mutex_)) {
+  CHECK_EQ(0, pthread_mutex_lock(&mutex_));
-    return false;
+  event_set_ = true;
-  }
+  pthread_cond_signal(&cond_);
-  state_ = kUp;
-  // Release all waiting threads
-  pthread_cond_broadcast(&cond_);
   pthread_mutex_unlock(&mutex_);
   return true;
 }
 
 EventTypeWrapper EventPosix::Wait(unsigned long timeout) {
   int ret_val = 0;
-  if (0 != pthread_mutex_lock(&mutex_)) {
+  CHECK_EQ(0, pthread_mutex_lock(&mutex_));
-    return kEventError;
-  }
 
-  if (kDown == state_) {
+  if (!event_set_) {
     if (WEBRTC_EVENT_INFINITE != timeout) {
       timespec end_at;
 #ifndef WEBRTC_MAC
@@ -133,52 +95,43 @@ EventTypeWrapper EventPosix::Wait(unsigned long timeout) {
         end_at.tv_sec++;
         end_at.tv_nsec -= E9;
       }
-      ret_val = pthread_cond_timedwait(&cond_, &mutex_, &end_at);
+      while (ret_val == 0 && !event_set_)
+        ret_val = pthread_cond_timedwait(&cond_, &mutex_, &end_at);
     } else {
-      ret_val = pthread_cond_wait(&cond_, &mutex_);
+      while (ret_val == 0 && !event_set_)
+        ret_val = pthread_cond_wait(&cond_, &mutex_);
     }
   }
 
-  // Be careful to only change the state if we're about to report that the
+  DCHECK(ret_val == 0 || ret_val == ETIMEDOUT);
-  // event was signaled.
-  if (ret_val == 0) {
-    // state_ might already be kDown, in case of multiple waiters. That's OK.
-    state_ = kDown;
-  }
 
+  // Reset and signal if set, regardless of why the thread woke up.
+  if (event_set_) {
+    ret_val = 0;
+    event_set_ = false;
+  }
   pthread_mutex_unlock(&mutex_);
 
-  switch (ret_val) {
+  return ret_val == 0 ? kEventSignaled : kEventTimeout;
-    case 0:
-      return kEventSignaled;
-    case ETIMEDOUT:
-      return kEventTimeout;
-    default:
-      return kEventError;
-  }
 }
 
-EventTypeWrapper EventPosix::Wait(timespec& wake_at) {
+EventTypeWrapper EventPosix::Wait(timespec* end_at) {
   int ret_val = 0;
-  if (0 != pthread_mutex_lock(&mutex_)) {
+  CHECK_EQ(0, pthread_mutex_lock(&mutex_));
-    return kEventError;
-  }
 
-  if (kUp != state_) {
+  while (ret_val == 0 && !event_set_)
-    ret_val = pthread_cond_timedwait(&cond_, &mutex_, &wake_at);
+    ret_val = pthread_cond_timedwait(&cond_, &mutex_, end_at);
-  }
-  state_ = kDown;
 
+  DCHECK(ret_val == 0 || ret_val == ETIMEDOUT);
+
+  // Reset and signal if set, regardless of why the thread woke up.
+  if (event_set_) {
+    ret_val = 0;
+    event_set_ = false;
+  }
   pthread_mutex_unlock(&mutex_);
 
-  switch (ret_val) {
+  return ret_val == 0 ? kEventSignaled : kEventTimeout;
-    case 0:
-      return kEventSignaled;
-    case ETIMEDOUT:
-      return kEventTimeout;
-    default:
-      return kEventError;
-  }
 }
 
 bool EventPosix::StartTimer(bool periodic, unsigned long time) {
@@ -246,14 +199,8 @@ bool EventPosix::Process() {
   }
 
   pthread_mutex_unlock(&mutex_);
-  switch (timer_event_->Wait(end_at)) {
+  if (timer_event_->Wait(&end_at) == kEventSignaled)
-    case kEventSignaled:
+    return true;
-      return true;
-    case kEventError:
-      return false;
-    case kEventTimeout:
-      break;
-  }
 
   pthread_mutex_lock(&mutex_);
   if (periodic_ || count_ == 1)

webrtc/system_wrappers/source/event_posix.h

@@ -39,15 +39,15 @@ class EventPosix : public EventWrapper {
 
  private:
   EventPosix();
-  int Construct();
 
   static bool Run(ThreadObj obj);
   bool Process();
-  EventTypeWrapper Wait(timespec& wake_at);
+  EventTypeWrapper Wait(timespec* end_at);
 
  private:
   pthread_cond_t  cond_;
   pthread_mutex_t mutex_;
+  bool event_set_;
 
   ThreadWrapper* timer_thread_;
   EventPosix*    timer_event_;
@@ -56,7 +56,6 @@ class EventPosix : public EventWrapper {
   bool          periodic_;
   unsigned long time_;  // In ms
   unsigned long count_;
-  State         state_;
 };
 
 }  // namespace webrtc