timers: assorted cleanup + improvements

lib/internal/timers.js

@@ -85,7 +85,6 @@ Timeout.prototype[refreshFnSymbol] = function refresh() {
     // Would be more ideal with uv_timer_again(), however that API does not
     // cause libuv's sorted timers data structure (a binary heap at the time
     // of writing) to re-sort itself. This causes ordering inconsistencies.
-    this._handle.stop();
     this._handle.start(this._idleTimeout);
   } else if (this[unrefedSymbol]) {
     getTimers()._unrefActive(this);

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;
@@ -222,19 +221,11 @@ function TimersList(msecs, unrefed) {
 
 // adds listOnTimeout to the C++ object prototype, as
 // V8 would not inline it otherwise.
-TimerWrap.prototype[kOnTimeout] = function listOnTimeout() {
-  var list = this._list;
-  var msecs = list.msecs;
-
-  if (list.nextTick) {
-    list.nextTick = false;
-    process.nextTick(listOnTimeoutNT, list);
-    return;
-  }
+TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) {
+  const list = this._list;
+  const msecs = list.msecs;
 
   debug('timeout callback %d', msecs);
-
-  var now = TimerWrap.now();
   debug('now: %d', now);
 
   var diff, timer;
@@ -245,12 +236,12 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() {
     // This happens if there are more timers scheduled for later in the list.
     if (diff < msecs) {
       var timeRemaining = msecs - (TimerWrap.now() - timer._idleStart);
-      if (timeRemaining < 0) {
+      if (timeRemaining <= 0) {
         timeRemaining = 1;
       }
       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 +278,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 +307,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.
@@ -353,7 +320,7 @@ function listOnTimeoutNT(list) {
 function reuse(item) {
   L.remove(item);
 
-  var list = refedLists[item._idleTimeout];
+  const list = refedLists[item._idleTimeout];
   // if empty - reuse the watcher
   if (list !== undefined && L.isEmpty(list)) {
     debug('reuse hit');
@@ -377,7 +344,7 @@ const unenroll = exports.unenroll = function(item) {
     item._destroyed = true;
   }
 
-  var handle = reuse(item);
+  const handle = reuse(item);
   if (handle !== null) {
     debug('unenroll: list empty');
     handle.close();
@@ -449,11 +416,12 @@ setTimeout[internalUtil.promisify.custom] = function(after, value) {
 exports.setTimeout = setTimeout;
 
 
-function ontimeout(timer) {
-  var args = timer._timerArgs;
+function ontimeout(timer, start) {
+  const args = timer._timerArgs;
   if (typeof timer._onTimeout !== 'function')
     return promiseResolve(timer._onTimeout, args[0]);
-  const start = TimerWrap.now();
+  if (start === undefined && timer._repeat)
+    start = TimerWrap.now();
   if (!args)
     timer._onTimeout();
   else
@@ -537,26 +505,30 @@ 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);
+function unrefdHandle(now) {
+  try {
+    // Don't attempt to call the callback if it is not a function.
+    if (typeof this.owner._onTimeout === 'function') {
+      ontimeout(this.owner, now);
+    }
+  } 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;
 }
 
 
 Timeout.prototype.unref = function() {
   if (this._handle) {
     this._handle.unref();
   } else if (typeof this._onTimeout === 'function') {
-    var now = TimerWrap.now();
+    const now = TimerWrap.now();
     if (!this._idleStart) this._idleStart = now;
     var delay = this._idleStart + this._idleTimeout - now;
     if (delay < 0) delay = 0;
@@ -567,7 +539,7 @@ Timeout.prototype.unref = function() {
       return;
     }
 
-    var handle = reuse(this);
+    const handle = reuse(this);
     if (handle !== null) {
       handle._list = undefined;
     }

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);

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
     };
 

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;
   }
 }

src/timer_wrap.cc

@@ -37,6 +37,7 @@ using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Local;
+using v8::Number;
 using v8::Object;
 using v8::String;
 using v8::Value;
@@ -138,19 +139,32 @@ 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;
+    Local<Value> args[1];
+    do {
+      args[0] = GetNow(env);
+      ret = wrap->MakeCallback(kOnTimeout, 1, args).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) {
     Environment* env = Environment::GetCurrent(args);
+    args.GetReturnValue().Set(GetNow(env));
+  }
+
+  static Local<Value> GetNow(Environment* env) {
     uv_update_time(env->event_loop());
     uint64_t now = uv_now(env->event_loop());
     CHECK(now >= env->timer_base());
     now -= env->timer_base();
     if (now <= 0xfffffff)
-      args.GetReturnValue().Set(static_cast<uint32_t>(now));
+      return Integer::New(env->isolate(), static_cast<uint32_t>(now));
     else
-      args.GetReturnValue().Set(static_cast<double>(now));
+      return Number::New(env->isolate(), static_cast<double>(now));
   }
 
   uv_timer_t handle_;

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);

test/sequential/test-timers-set-interval-excludes-callback-duration.js

@@ -9,9 +9,16 @@ const t = setInterval(() => {
   cntr++;
   if (cntr === 1) {
     common.busyLoop(100);
+    // ensure that the event loop passes before the second interval
+    setImmediate(() => assert.strictEqual(cntr, 1));
     first = Timer.now();
   } else if (cntr === 2) {
     assert(Timer.now() - first < 100);
     clearInterval(t);
   }
 }, 100);
+const t2 = setInterval(() => {
+  if (cntr === 2) {
+    clearInterval(t2);
+  }
+}, 100);