timers: refactor error handling

Instead of using nextTick to process failed lists, just attempt to process them again from C++ if the process is still alive. This also allows the removal of domain specific code in timers. The current behaviour is not quite ideal as it means that all lists after the failed one will process on an arbitrary nextTick, even if they're — say — not due to fire for another 2 days...
apapirovski · Jan 29, 2018 · Jan 31, 2018 · Jan 31, 2018 · Jan 31, 2018 · Jan 31, 2018
commit 0f0304b4aea3bb89d70dfd3a8e1dabdfcca034f1
diff --git a/lib/timers.js b/lib/timers.js
@@ -210,7 +210,6 @@ function TimersList(msecs, unrefed) {
   this._idlePrev = this; // prevent any unnecessary hidden class changes.
   this._unrefed = unrefed;
   this.msecs = msecs;
-  this.nextTick = false;
 
   const timer = this._timer = new TimerWrap();
   timer._list = this;
@@ -226,12 +225,6 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() {
   var list = this._list;
   var msecs = list.msecs;
 
-  if (list.nextTick) {
-    list.nextTick = false;
-    process.nextTick(listOnTimeoutNT, list);
-    return;
-  }
-
   debug('timeout callback %d', msecs);
 
   var now = TimerWrap.now();
@@ -250,7 +243,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() {
       }
       this.start(timeRemaining);
       debug('%d list wait because diff is %d', msecs, diff);
-      return;
+      return true;
     }
 
     // The actual logic for when a timeout happens.
@@ -287,10 +280,10 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() {
 
   // Do not close the underlying handle if its ownership has changed
   // (e.g it was unrefed in its callback).
-  if (this.owner)
-    return;
+  if (!this.owner)
+    this.close();
 
-  this.close();
+  return true;
 };
 
 
@@ -316,34 +309,10 @@ function tryOnTimeout(timer, list) {
         timer._destroyed = true;
       }
     }
-
-    if (!threw) return;
-
-    // Postpone all later list events to next tick. We need to do this
-    // so that the events are called in the order they were created.
-    const lists = list._unrefed === true ? unrefedLists : refedLists;
-    for (var key in lists) {
-      if (key > list.msecs) {
-        lists[key].nextTick = true;
-      }
-    }
-    // We need to continue processing after domain error handling
-    // is complete, but not by using whatever domain was left over
-    // when the timeout threw its exception.
-    const domain = process.domain;
-    process.domain = null;
-    // If we threw, we need to process the rest of the list in nextTick.
-    process.nextTick(listOnTimeoutNT, list);
-    process.domain = domain;
   }
 }
 
 
-function listOnTimeoutNT(list) {
-  list._timer[kOnTimeout]();
-}
-
-
 // A convenience function for re-using TimerWrap handles more easily.
 //
 // This mostly exists to fix https://github.com/nodejs/node/issues/1264.
@@ -538,17 +507,21 @@ exports.clearInterval = function(timer) {
 
 
 function unrefdHandle() {
-  // Don't attempt to call the callback if it is not a function.
-  if (typeof this.owner._onTimeout === 'function') {
-    ontimeout(this.owner);
+  try {
+    // Don't attempt to call the callback if it is not a function.
+    if (typeof this.owner._onTimeout === 'function') {
+      ontimeout(this.owner);
+    }
+  } finally {
+    // Make sure we clean up if the callback is no longer a function
+    // even if the timer is an interval.
+    if (!this.owner._repeat ||
+        typeof this.owner._onTimeout !== 'function') {
+      this.owner.close();
+    }
   }
 
-  // Make sure we clean up if the callback is no longer a function
-  // even if the timer is an interval.
-  if (!this.owner._repeat ||
-      typeof this.owner._onTimeout !== 'function') {
-    this.owner.close();
-  }
+  return true;
 }
 
 

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -264,6 +264,10 @@ inline bool Environment::TickInfo::has_scheduled() const {
   return fields_[kHasScheduled] == 1;
 }
 
+inline bool Environment::TickInfo::has_thrown() const {
+  return fields_[kHasThrown] == 1;
+}
+
 inline bool Environment::TickInfo::has_promise_rejections() const {
   return fields_[kHasPromiseRejections] == 1;
 }
@@ -272,6 +276,10 @@ inline void Environment::TickInfo::promise_rejections_toggle_on() {
   fields_[kHasPromiseRejections] = 1;
 }
 
+inline void Environment::TickInfo::set_has_thrown(bool state) {
+  fields_[kHasThrown] = state ? 1 : 0;
+}
+
 inline void Environment::AssignToContext(v8::Local<v8::Context> context,
                                          const ContextInfo& info) {
   context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this);

diff --git a/src/env.h b/src/env.h
@@ -479,8 +479,10 @@ class Environment {
     inline AliasedBuffer<uint8_t, v8::Uint8Array>& fields();
     inline bool has_scheduled() const;
     inline bool has_promise_rejections() const;
+    inline bool has_thrown() const;
 
     inline void promise_rejections_toggle_on();
+    inline void set_has_thrown(bool state);
 
    private:
     friend class Environment;  // So we can call the constructor.
@@ -489,6 +491,7 @@ class Environment {
     enum Fields {
       kHasScheduled,
       kHasPromiseRejections,
+      kHasThrown,
       kFieldsCount
     };
 

diff --git a/src/node.cc b/src/node.cc
@@ -937,6 +937,10 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
     AsyncWrap::EmitBefore(env, asyncContext.async_id);
   }
 
+  if (!IsInnerMakeCallback()) {
+    env->tick_info()->set_has_thrown(false);
+  }
+
   env->async_hooks()->push_async_ids(async_context_.async_id,
                                async_context_.trigger_async_id);
   pushed_ids_ = true;
@@ -984,6 +988,7 @@ void InternalCallbackScope::Close() {
   Local<Object> process = env_->process_object();
 
   if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
+    env_->tick_info()->set_has_thrown(true);
     failed_ = true;
   }
 }

diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc
@@ -138,7 +138,14 @@ class TimerWrap : public HandleWrap {
     Environment* env = wrap->env();
     HandleScope handle_scope(env->isolate());
     Context::Scope context_scope(env->context());
-    wrap->MakeCallback(kOnTimeout, 0, nullptr);
+    Local<Value> ret;
+    do {
+      ret = wrap->MakeCallback(kOnTimeout, 0, nullptr).ToLocalChecked();
+    } while (ret->IsUndefined() &&
+             !env->tick_info()->has_thrown() &&
+             wrap->object()->Get(env->context(),
+                                 env->owner_string()).ToLocalChecked()
+                                                     ->IsUndefined());
   }
 
   static void Now(const FunctionCallbackInfo<Value>& args) {

diff --git a/test/parallel/test-timers-unref-throw-then-ref.js b/test/parallel/test-timers-unref-throw-then-ref.js
@@ -0,0 +1,17 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+process.once('uncaughtException', common.expectsError({
+  message: 'Timeout Error'
+}));
+
+let called = false;
+const t = setTimeout(() => {
+  assert(!called);
+  called = true;
+  t.ref();
+  throw new Error('Timeout Error');
+}, 1).unref();
+
+setTimeout(common.mustCall(), 1);