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

FutexEmulation::Wait can potentially block forever on a condition variable. We
want to allow this to be interrupted (for a debugger, or to terminate the
thread, for example).

The previous implementation would periodically wake up the waiter to check for
interrupts. This CL modifies the StackGuard so it wakes the blocked futex if
the thread should be interrupted.

BUG=chromium:497295
R=jarin@chromium.org
LOG=n

Review URL: https://codereview.chromium.org/1230303005

Cr-Commit-Position: refs/heads/master@{#30311}

src/execution.cc

@@ -413,6 +413,9 @@ void StackGuard::RequestInterrupt(InterruptFlag flag) {
   // Not intercepted.  Set as active interrupt flag.
   thread_local_.interrupt_flags_ |= flag;
   set_interrupt_limits(access);
+
+  // If this isolate is waiting in a futex, notify it to wake up.
+  isolate_->futex_wait_list_node()->NotifyWake();
 }
 
 

src/futex-emulation.cc

@@ -21,6 +21,23 @@ 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());
+  if (waiting_) {
+    cond_.NotifyOne();
+    interrupted_ = true;
+  }
+}
+
+
 FutexWaitList::FutexWaitList() : head_(nullptr), tail_(nullptr) {}
 
 
@@ -58,12 +75,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();
@@ -103,41 +114,75 @@ Object* FutexEmulation::Wait(Isolate* isolate,
 
   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::TimeTicks current_time = base::TimeTicks::Now();
-    if (use_timeout && current_time > timeout_time) {
-      result = Smi::FromInt(Result::kTimedOut);
-      break;
+    bool interrupted = node->interrupted_;
+    node->interrupted_ = false;
+
+    // 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 (interrupted) {
+      Object* interrupt_object = isolate->stack_guard()->HandleInterrupts();
+      if (interrupt_object->IsException()) {
+        result = interrupt_object;
+        mutex_.Pointer()->Lock();
+        break;
+      }
     }
 
-    base::TimeDelta time_until_timeout = timeout_time - current_time;
-    base::TimeDelta time_to_wait =
-        (use_timeout && time_until_timeout < kMaxWaitTime) ? time_until_timeout
-                                                           : kMaxWaitTime;
+    mutex_.Pointer()->Lock();
 
-    bool wait_for_result = node->cond_.WaitFor(mutex_.Pointer(), time_to_wait);
-    USE(wait_for_result);
+    if (node->interrupted_) {
+      // An interrupt occured while the mutex_ was unlocked. Don't wait yet.
+      continue;
+    }
 
     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;
+    // No interrupts, now wait.
+    if (use_timeout) {
+      current_time = base::TimeTicks::Now();
+      if (current_time >= timeout_time) {
+        result = Smi::FromInt(Result::kTimedOut);
+        break;
+      }
+
+      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());
     }
+
+    // Spurious wakeup, interrupt or timeout.
   }
 
   wait_list_.Pointer()->RemoveNode(node);
+  node->waiting_ = false;
 
   return result;
 }

src/futex-emulation.h

@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include "src/allocation.h"
+#include "src/base/atomicops.h"
 #include "src/base/lazy-instance.h"
 #include "src/base/macros.h"
 #include "src/base/platform/condition-variable.h"
@@ -40,7 +41,10 @@ class FutexWaitListNode {
         next_(nullptr),
         backing_store_(nullptr),
         wait_addr_(0),
-        waiting_(false) {}
+        waiting_(false),
+        interrupted_(false) {}
+
+  void NotifyWake();
 
  private:
   friend class FutexEmulation;
@@ -52,6 +56,7 @@ class FutexWaitListNode {
   void* backing_store_;
   size_t wait_addr_;
   bool waiting_;
+  bool interrupted_;
 
   DISALLOW_COPY_AND_ASSIGN(FutexWaitListNode);
 };
@@ -115,6 +120,8 @@ class FutexEmulation : public AllStatic {
                                       size_t addr);
 
  private:
+  friend class FutexWaitListNode;
+
   static base::LazyMutex mutex_;
   static base::LazyInstance<FutexWaitList>::type wait_list_;
 };

test/cctest/test-api.cc

@@ -21848,6 +21848,7 @@ TEST(FutexInterruption) {
       "var i32a = new Int32Array(ab);"
       "Atomics.futexWait(i32a, 0, 0);");
   CHECK(try_catch.HasTerminated());
+  timeout_thread.Join();
 }