Signal a blocked futex if the isolate is interrupted; don't busy-wait

src/futex-emulation.cc

Index: src/futex-emulation.cc
diff --git a/src/futex-emulation.cc b/src/futex-emulation.cc
index 39f43dd7dd8c998e8ec571b5942194827a240f75..2c9d177304afabc66eb318c412488138af50a13e 100644
--- a/src/futex-emulation.cc
+++ b/src/futex-emulation.cc
@@ -21,6 +21,21 @@ base::LazyInstance<FutexWaitList>::type FutexEmulation::wait_list_ =
     LAZY_INSTANCE_INITIALIZER;
 
 
+void FutexWaitListNode::NotifyWake() {
+  // Lock the FutexEmulation mutex before notifying. We know that the mutex
+  // will have been unlocked if we are currently waiting on the condition
+  // variable.
+  //
+  // The mutex may also not be locked if the other thread is currently handling
+  // interrupts, or if FutexEmulation::Wait was just called and the mutex
+  // hasn't been locked yet. In either of those cases, we set the interrupted
+  // flag to true, which will be tested after the mutex is re-locked.
+  base::LockGuard<base::Mutex> lock_guard(FutexEmulation::mutex_.Pointer());
+  cond_.NotifyOne();
+  set_interrupted(true);
+}
+
+
 FutexWaitList::FutexWaitList() : head_(nullptr), tail_(nullptr) {}
 
 
@@ -58,12 +73,6 @@ void FutexWaitList::RemoveNode(FutexWaitListNode* node) {
 Object* FutexEmulation::Wait(Isolate* isolate,
                              Handle<JSArrayBuffer> array_buffer, size_t addr,
                              int32_t value, double rel_timeout_ms) {
-  // We never want to wait longer than this amount of time; this way we can
-  // interrupt this thread even if this is an "infinitely blocking" wait.
-  // TODO(binji): come up with a better way of interrupting only when
-  // necessary, rather than busy-waiting.
-  const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromMilliseconds(50);
-
   DCHECK(addr < NumberToSize(isolate, array_buffer->byte_length()));
 
   void* backing_store = array_buffer->backing_store();
@@ -80,7 +89,7 @@ Object* FutexEmulation::Wait(Isolate* isolate,
 
   node->backing_store_ = backing_store;
   node->wait_addr_ = addr;
-  node->waiting_ = true;
+  node->set_waiting(true);
 
   bool use_timeout = rel_timeout_ms != V8_INFINITY;
 
@@ -101,40 +110,70 @@ Object* FutexEmulation::Wait(Isolate* isolate,
     }
   }
 
-  base::Time start_time = base::Time::NowFromSystemTime();
-  base::Time timeout_time = start_time + rel_timeout;
+  base::TimeTicks start_time = base::TimeTicks::Now();
+  base::TimeTicks timeout_time = start_time + rel_timeout;
+  base::TimeTicks current_time = start_time;
 
   wait_list_.Pointer()->AddNode(node);
 
   Object* result;
 
   while (true) {
-    base::Time current_time = base::Time::NowFromSystemTime();
-    if (use_timeout && current_time > timeout_time) {
-      result = Smi::FromInt(Result::kTimedOut);
-      break;
+    // Unlock the mutex here to prevent deadlock from lock ordering between
+    // mutex_ and mutexes locked by HandleInterrupts.
+    mutex_.Pointer()->Unlock();
+
+    // Because the mutex is unlocked, we have to be careful about not dropping
+    // an interrupt. The notification can happen in three different places:
+    // 1) Before Wait is called: the notification will be dropped, but
+    //    interrupted_ will be set to 1. This will be checked below.
+    // 2) After interrupted has been checked here, but before mutex_ is
+    //    acquired: interrupted is checked again below, with mutex_ locked.
+    //    Because the wakeup signal also acquires mutex_, we know it will not
+    //    be able to notify until mutex_ is released below, when waiting on the
+    //    condition variable.
+    // 3) After the mutex is released in the call to WaitFor(): this
+    // notification will wake up the condition variable. node->waiting() will
+    // be false, so we'll loop and then check interrupts.
+    if (node->CheckInterruptedAndClear()) {

[Jarin, 2015/08/11 08:21:49]

Why can't you read and clear the interrupt flag while still holding the lock?
That way, you would not have to resort to the CASing, no? (You could also use
lexically scoped guards.)

[binji, 2015/08/11 14:51:17]

Good point, I'll change this.

+      Object* interrupt_object = isolate->stack_guard()->HandleInterrupts();
+      if (interrupt_object->IsException()) {
+        result = interrupt_object;
+        mutex_.Pointer()->Lock();
+        break;
+      }
+    }
+
+    mutex_.Pointer()->Lock();
+
+    if (node->interrupted()) {
+      // An interrupt occured while the mutex_ was unlocked. Don't wait yet.
+      continue;
     }
 
-    base::TimeDelta time_until_timeout = timeout_time - current_time;
-    base::TimeDelta time_to_wait =
-        (use_timeout && time_until_timeout < kMaxWaitTime) ? time_until_timeout
-                                                           : kMaxWaitTime;
+    // No interrupts, now wait.
+    if (use_timeout) {
+      current_time = base::TimeTicks::Now();
+      if (current_time >= timeout_time) {
+        result = Smi::FromInt(Result::kTimedOut);
+        break;
+      }
 
-    bool wait_for_result = node->cond_.WaitFor(mutex_.Pointer(), time_to_wait);
-    USE(wait_for_result);
+      base::TimeDelta time_until_timeout = timeout_time - current_time;
+      DCHECK(time_until_timeout.InMicroseconds() >= 0);
+      bool wait_for_result =
+          node->cond_.WaitFor(mutex_.Pointer(), time_until_timeout);
+      USE(wait_for_result);
+    } else {
+      node->cond_.Wait(mutex_.Pointer());
+    }
 
-    if (!node->waiting_) {
+    if (!node->waiting()) {
       result = Smi::FromInt(Result::kOk);
       break;
     }
 
-    // Spurious wakeup or timeout. Potentially handle interrupts before
-    // continuing to wait.
-    Object* interrupt_object = isolate->stack_guard()->HandleInterrupts();
-    if (interrupt_object->IsException()) {
-      result = interrupt_object;
-      break;
-    }
+    // Spurious wakeup, interrupt or timeout.
   }
 
   wait_list_.Pointer()->RemoveNode(node);
@@ -155,7 +194,7 @@ Object* FutexEmulation::Wake(Isolate* isolate,
   FutexWaitListNode* node = wait_list_.Pointer()->head_;
   while (node && num_waiters_to_wake > 0) {
     if (backing_store == node->backing_store_ && addr == node->wait_addr_) {
-      node->waiting_ = false;
+      node->set_waiting(false);
       node->cond_.NotifyOne();
       --num_waiters_to_wake;
       waiters_woken++;
@@ -190,7 +229,7 @@ Object* FutexEmulation::WakeOrRequeue(Isolate* isolate,
   while (node) {
     if (backing_store == node->backing_store_ && addr == node->wait_addr_) {
       if (num_waiters_to_wake > 0) {
-        node->waiting_ = false;
+        node->set_waiting(false);
         node->cond_.NotifyOne();
         --num_waiters_to_wake;
         waiters_woken++;