timers: use only a single TimerWrap instance

Hang all timer lists off a single TimerWrap and use the PriorityQueue
to manage expiration priorities. This makes the Timers code clearer,
consumes significantly less resources and improves performance.

PR-URL: https://github.com/nodejs/node/pull/20555
Fixes: https://github.com/nodejs/node/issues/16105
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

benchmark/timers/timers-cancel-unpooled.js

@@ -4,18 +4,26 @@ const assert = require('assert');
 
 const bench = common.createBenchmark(main, {
   n: [1e6],
+  direction: ['start', 'end']
 });
 
-function main({ n }) {
+function main({ n, direction }) {
 
   const timersList = [];
   for (var i = 0; i < n; i++) {
     timersList.push(setTimeout(cb, i + 1));
   }
 
+  var j;
   bench.start();
-  for (var j = 0; j < n + 1; j++) {
-    clearTimeout(timersList[j]);
+  if (direction === 'start') {
+    for (j = 0; j < n; j++) {
+      clearTimeout(timersList[j]);
+    }
+  } else {
+    for (j = n - 1; j >= 0; j--) {
+      clearTimeout(timersList[j]);
+    }
   }
   bench.end(n);
 }

benchmark/timers/timers-insert-unpooled.js

@@ -4,18 +4,26 @@ const assert = require('assert');
 
 const bench = common.createBenchmark(main, {
   n: [1e6],
+  direction: ['start', 'end']
 });
 
-function main({ n }) {
+function main({ direction, n }) {
   const timersList = [];
 
+  var i;
   bench.start();
-  for (var i = 0; i < n; i++) {
-    timersList.push(setTimeout(cb, i + 1));
+  if (direction === 'start') {
+    for (i = 1; i <= n; i++) {
+      timersList.push(setTimeout(cb, i));
+    }
+  } else {
+    for (i = n; i > 0; i--) {
+      timersList.push(setTimeout(cb, i));
+    }
   }
   bench.end(n);
 
-  for (var j = 0; j < n + 1; j++) {
+  for (var j = 0; j < n; j++) {
     clearTimeout(timersList[j]);
   }
 }

lib/internal/timers.js

@@ -19,7 +19,7 @@ const {
 // Timeout values > TIMEOUT_MAX are set to 1.
 const TIMEOUT_MAX = 2 ** 31 - 1;
 
-const unrefedSymbol = Symbol('unrefed');
+const kRefed = Symbol('refed');
 
 module.exports = {
   TIMEOUT_MAX,
@@ -27,6 +27,7 @@ module.exports = {
   async_id_symbol,
   trigger_async_id_symbol,
   Timeout,
+  kRefed,
   initAsyncResource,
   setUnrefTimeout,
   validateTimerDuration
@@ -50,7 +51,7 @@ function initAsyncResource(resource, type) {
 
 // Timer constructor function.
 // The entire prototype is defined in lib/timers.js
-function Timeout(callback, after, args, isRepeat, isUnrefed) {
+function Timeout(callback, after, args, isRepeat) {
   after *= 1; // coalesce to number or NaN
   if (!(after >= 1 && after <= TIMEOUT_MAX)) {
     if (after > TIMEOUT_MAX) {
@@ -62,7 +63,6 @@ function Timeout(callback, after, args, isRepeat, isUnrefed) {
     after = 1; // schedule on next tick, follows browser behavior
   }
 
-  this._called = false;
   this._idleTimeout = after;
   this._idlePrev = this;
   this._idleNext = this;
@@ -75,22 +75,16 @@ function Timeout(callback, after, args, isRepeat, isUnrefed) {
   this._repeat = isRepeat ? after : null;
   this._destroyed = false;
 
-  this[unrefedSymbol] = isUnrefed;
+  this[kRefed] = null;
 
   initAsyncResource(this, 'Timeout');
 }
 
 Timeout.prototype.refresh = function() {
-  if (this._handle) {
-    // 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.start(this._idleTimeout);
-  } else if (this[unrefedSymbol]) {
-    getTimers()._unrefActive(this);
-  } else {
+  if (this[kRefed])
     getTimers().active(this);
-  }
+  else
+    getTimers()._unrefActive(this);
 
   return this;
 };
@@ -122,7 +116,7 @@ function setUnrefTimeout(callback, after, arg1, arg2, arg3) {
       break;
   }
 
-  const timer = new Timeout(callback, after, args, false, true);
+  const timer = new Timeout(callback, after, args, false);
   getTimers()._unrefActive(timer);
 
   return timer;

lib/timers.js

@@ -26,16 +26,17 @@ const {
   setupTimers,
 } = process.binding('timer_wrap');
 const L = require('internal/linkedlist');
+const PriorityQueue = require('internal/priority_queue');
 const {
   async_id_symbol,
   trigger_async_id_symbol,
   Timeout,
+  kRefed,
   initAsyncResource,
   validateTimerDuration
 } = require('internal/timers');
 const internalUtil = require('internal/util');
 const { createPromise, promiseResolve } = process.binding('util');
-const assert = require('assert');
 const util = require('util');
 const { ERR_INVALID_CALLBACK } = require('internal/errors').codes;
 const debug = util.debuglog('timer');
@@ -55,8 +56,6 @@ const kHasOutstanding = 2;
 const [immediateInfo, toggleImmediateRef] =
   setupTimers(processImmediate, processTimers);
 
-const kRefed = Symbol('refed');
-
 // HOW and WHY the timers implementation works the way it does.
 //
 // Timers are crucial to Node.js. Internally, any TCP I/O connection creates a
@@ -85,20 +84,17 @@ const kRefed = Symbol('refed');
 //
 // Object maps are kept which contain linked lists keyed by their duration in
 // milliseconds.
-// The linked lists within also have some meta-properties, one of which is a
-// TimerWrap C++ handle, which makes the call after the duration to process the
-// list it is attached to.
 //
 /* eslint-disable node-core/non-ascii-character */
 //
 // ╔════ > Object Map
 // ║
 // ╠══
-// ║ refedLists: { '40': { }, '320': { etc } } (keys of millisecond duration)
-// ╚══          ┌─────────┘
+// ║ lists: { '40': { }, '320': { etc } } (keys of millisecond duration)
+// ╚══          ┌────┘
 //              │
 // ╔══          │
-// ║ TimersList { _idleNext: { }, _idlePrev: (self), _timer: (TimerWrap) }
+// ║ TimersList { _idleNext: { }, _idlePrev: (self) }
 // ║         ┌────────────────┘
 // ║    ╔══  │                              ^
 // ║    ║    { _idleNext: { },  _idlePrev: { }, _onTimeout: (callback) }
@@ -126,36 +122,73 @@ const kRefed = Symbol('refed');
 // always be due to timeout at a later time.
 //
 // Less-than constant time operations are thus contained in two places:
-// TimerWrap's backing libuv timers implementation (a performant heap-based
-// queue), and the object map lookup of a specific list by the duration of
-// timers within (or creation of a new list).
-// However, these operations combined have shown to be trivial in comparison to
-// other alternative timers architectures.
+// The PriorityQueue — an efficient binary heap implementation that does all
+// operations in worst-case O(log n) time — which manages the order of expiring
+// Timeout lists and the object map lookup of a specific list by the duration of
+// timers within (or creation of a new list). However, these operations combined
+// have shown to be trivial in comparison to other timers architectures.
 
 
-// Object maps containing linked lists of timers, keyed and sorted by their
+// Object map containing linked lists of timers, keyed and sorted by their
 // duration in milliseconds.
 //
-// The difference between these two objects is that the former contains timers
-// that will keep the process open if they are the only thing left, while the
-// latter will not.
-//
 // - key = time in milliseconds
 // - value = linked list
-const refedLists = Object.create(null);
-const unrefedLists = Object.create(null);
+const lists = Object.create(null);
 
+// This is a priority queue with a custom sorting function that first compares
+// the expiry times of two lists and if they're the same then compares their
+// individual IDs to determine which list was created first.
+const queue = new PriorityQueue(compareTimersLists, setPosition);
+
+function compareTimersLists(a, b) {
+  const expiryDiff = a.expiry - b.expiry;
+  if (expiryDiff === 0) {
+    if (a.id < b.id)
+      return -1;
+    if (a.id > b.id)
+      return 1;
+  }
+  return expiryDiff;
+}
+
+function setPosition(node, pos) {
+  node.priorityQueuePosition = pos;
+}
+
+let handle = null;
+let nextExpiry = Infinity;
+
+let timerListId = Number.MIN_SAFE_INTEGER;
+let refCount = 0;
+
+function incRefCount() {
+  if (refCount++ === 0)
+    handle.ref();
+}
+
+function decRefCount() {
+  if (--refCount === 0)
+    handle.unref();
+}
+
+function createHandle(refed) {
+  debug('initial run, creating TimerWrap handle');
+  handle = new TimerWrap();
+  if (!refed)
+    handle.unref();
+}
 
 // Schedule or re-schedule a timer.
 // The item must have been enroll()'d first.
 const active = exports.active = function(item) {
-  insert(item, false);
+  insert(item, true, TimerWrap.now());
 };
 
 // Internal APIs that need timeouts should use `_unrefActive()` instead of
 // `active()` so that they do not unnecessarily keep the process open.
 exports._unrefActive = function(item) {
-  insert(item, true);
+  insert(item, false, TimerWrap.now());
 };
 
 
@@ -164,23 +197,27 @@ exports._unrefActive = function(item) {
 // Appends a timer onto the end of an existing timers list, or creates a new
 // TimerWrap backed list if one does not already exist for the specified timeout
 // duration.
-function insert(item, unrefed, start) {
+function insert(item, refed, start) {
   const msecs = item._idleTimeout;
-  if (msecs < 0 || msecs === undefined) return;
+  if (msecs < 0 || msecs === undefined)
+    return;
 
-  if (typeof start === 'number') {
-    item._idleStart = start;
-  } else {
-    item._idleStart = TimerWrap.now();
-  }
-
-  const lists = unrefed === true ? unrefedLists : refedLists;
+  item._idleStart = start;
 
   // Use an existing list if there is one, otherwise we need to make a new one.
   var list = lists[msecs];
   if (list === undefined) {
     debug('no %d list was found in insert, creating a new one', msecs);
-    lists[msecs] = list = new TimersList(msecs, unrefed);
+    const expiry = start + msecs;
+    lists[msecs] = list = new TimersList(expiry, msecs);
+    queue.insert(list);
+
+    if (nextExpiry > expiry) {
+      if (handle === null)
+        createHandle(refed);
+      handle.start(msecs);
+      nextExpiry = expiry;
+    }
   }
 
   if (!item[async_id_symbol] || item._destroyed) {
@@ -188,32 +225,55 @@ function insert(item, unrefed, start) {
     initAsyncResource(item, 'Timeout');
   }
 
+  if (refed === !item[kRefed]) {
+    if (refed)
+      incRefCount();
+    else
+      decRefCount();
+  }
+  item[kRefed] = refed;
+
   L.append(list, item);
-  assert(!L.isEmpty(list)); // list is not empty
 }
 
-function TimersList(msecs, unrefed) {
+function TimersList(expiry, msecs) {
   this._idleNext = this; // Create the list with the linkedlist properties to
   this._idlePrev = this; // prevent any unnecessary hidden class changes.
-  this._unrefed = unrefed;
+  this.expiry = expiry;
+  this.id = timerListId++;
   this.msecs = msecs;
-
-  const timer = this._timer = new TimerWrap();
-  timer._list = this;
-
-  if (unrefed === true)
-    timer.unref();
-  timer.start(msecs);
+  this.priorityQueuePosition = null;
 }
 
+const { _tickCallback: runNextTicks } = process;
 function processTimers(now) {
-  if (this.owner)
-    return unrefdHandle(this.owner, now);
-  return listOnTimeout(this, now);
+  debug('process timer lists %d', now);
+  nextExpiry = Infinity;
+
+  let list, ran;
+  while (list = queue.peek()) {
+    if (list.expiry > now)
+      break;
+    if (ran)
+      runNextTicks();
+    listOnTimeout(list, now);
+    ran = true;
+  }
+
+  if (refCount > 0)
+    handle.ref();
+  else
+    handle.unref();
+
+  if (list !== undefined) {
+    nextExpiry = list.expiry;
+    handle.start(Math.max(nextExpiry - TimerWrap.now(), 1));
+  }
+
+  return true;
 }
 
-function listOnTimeout(handle, now) {
-  const list = handle._list;
+function listOnTimeout(list, now) {
   const msecs = list.msecs;
 
   debug('timeout callback %d', msecs);
@@ -226,78 +286,69 @@ function listOnTimeout(handle, now) {
     // Check if this loop iteration is too early for the next timer.
     // 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) {
-        timeRemaining = 1;
-      }
-      handle.start(timeRemaining);
+      list.expiry = timer._idleStart + msecs;
+      list.id = timerListId++;
+      queue.percolateDown(1);
       debug('%d list wait because diff is %d', msecs, diff);
-      return true;
+      return;
     }
 
     // The actual logic for when a timeout happens.
-
     L.remove(timer);
-    assert(timer !== L.peek(list));
+
+    const asyncId = timer[async_id_symbol];
 
     if (!timer._onTimeout) {
-      if (destroyHooksExist() && !timer._destroyed &&
-            typeof timer[async_id_symbol] === 'number') {
-        emitDestroy(timer[async_id_symbol]);
+      if (timer[kRefed])
+        refCount--;
+      timer[kRefed] = null;
+
+      if (destroyHooksExist() && !timer._destroyed) {
+        emitDestroy(asyncId);
         timer._destroyed = true;
       }
       continue;
     }
 
+    emitBefore(asyncId, timer[trigger_async_id_symbol]);
+
     tryOnTimeout(timer);
+
+    emitAfter(asyncId);
   }
 
   // If `L.peek(list)` returned nothing, the list was either empty or we have
   // called all of the timer timeouts.
-  // As such, we can remove the list and clean up the TimerWrap C++ handle.
+  // As such, we can remove the list from the object map and the PriorityQueue.
   debug('%d list empty', msecs);
-  assert(L.isEmpty(list));
 
-  // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
-  // recreated since the reference to `list` was created. Make sure they're
-  // the same instance of the list before destroying.
-  if (list._unrefed === true && list === unrefedLists[msecs]) {
-    delete unrefedLists[msecs];
-  } else if (list === refedLists[msecs]) {
-    delete refedLists[msecs];
+  // The current list may have been removed and recreated since the reference
+  // to `list` was created. Make sure they're the same instance of the list
+  // before destroying.
+  if (list === lists[msecs]) {
+    delete lists[msecs];
+    queue.shift();
   }
-
-  // Do not close the underlying handle if its ownership has changed
-  // (e.g it was unrefed in its callback).
-  if (!handle.owner)
-    handle.close();
-
-  return true;
 }
 
 
 // An optimization so that the try/finally only de-optimizes (since at least v8
 // 4.7) what is in this smaller function.
 function tryOnTimeout(timer, start) {
-  timer._called = true;
-  const timerAsyncId = (typeof timer[async_id_symbol] === 'number') ?
-    timer[async_id_symbol] : null;
-  var threw = true;
-  if (timerAsyncId !== null)
-    emitBefore(timerAsyncId, timer[trigger_async_id_symbol]);
   if (start === undefined && timer._repeat)
     start = TimerWrap.now();
   try {
     ontimeout(timer);
-    threw = false;
   } finally {
-    if (timerAsyncId !== null) {
-      if (!threw)
-        emitAfter(timerAsyncId);
-      if (timer._repeat) {
-        rearm(timer, start);
-      } else if (destroyHooksExist() && !timer._destroyed) {
-        emitDestroy(timerAsyncId);
+    if (timer._repeat) {
+      rearm(timer, start);
+    } else {
+      if (timer[kRefed])
+        refCount--;
+      timer[kRefed] = null;
+
+      if (destroyHooksExist() && !timer._destroyed) {
+        emitDestroy(timer[async_id_symbol]);
         timer._destroyed = true;
       }
     }
@@ -305,43 +356,35 @@ function tryOnTimeout(timer, start) {
 }
 
 
-// A convenience function for re-using TimerWrap handles more easily.
-//
-// This mostly exists to fix https://github.com/nodejs/node/issues/1264.
-// Handles in libuv take at least one `uv_run` to be registered as unreferenced.
-// Re-using an existing handle allows us to skip that, so that a second `uv_run`
-// will return no active handles, even when running `setTimeout(fn).unref()`.
-function reuse(item) {
-  L.remove(item);
-
-  const list = refedLists[item._idleTimeout];
-  // if empty - reuse the watcher
-  if (list !== undefined && L.isEmpty(list)) {
-    debug('reuse hit');
-    list._timer.stop();
-    delete refedLists[item._idleTimeout];
-    return list._timer;
-  }
-
-  return null;
-}
-
-
 // Remove a timer. Cancels the timeout and resets the relevant timer properties.
 function unenroll(item) {
   // Fewer checks may be possible, but these cover everything.
   if (destroyHooksExist() &&
-      typeof item[async_id_symbol] === 'number' &&
+      item[async_id_symbol] !== undefined &&
       !item._destroyed) {
     emitDestroy(item[async_id_symbol]);
     item._destroyed = true;
   }
 
-  const handle = reuse(item);
-  if (handle !== null) {
-    debug('unenroll: list empty');
-    handle.close();
+  L.remove(item);
+
+  // We only delete refed lists because unrefed ones are incredibly likely
+  // to come from http and be recreated shortly after.
+  // TODO: Long-term this could instead be handled by creating an internal
+  // clearTimeout that makes it clear that the list should not be deleted.
+  // That function could then be used by http and other similar modules.
+  if (item[kRefed]) {
+    const list = lists[item._idleTimeout];
+    if (list !== undefined && L.isEmpty(list)) {
+      debug('unenroll: list empty');
+      queue.removeAt(list.priorityQueuePosition);
+      delete lists[list.msecs];
+    }
+
+    decRefCount();
   }
+  item[kRefed] = null;
+
   // if active is called later, then we want to make sure not to insert again
   item._idleTimeout = -1;
 }
@@ -403,7 +446,7 @@ function setTimeout(callback, after, arg1, arg2, arg3) {
       break;
   }
 
-  const timeout = new Timeout(callback, after, args, false, false);
+  const timeout = new Timeout(callback, after, args, false);
   active(timeout);
 
   return timeout;
@@ -411,7 +454,7 @@ function setTimeout(callback, after, arg1, arg2, arg3) {
 
 setTimeout[internalUtil.promisify.custom] = function(after, value) {
   const promise = createPromise();
-  const timeout = new Timeout(promise, after, [value], false, false);
+  const timeout = new Timeout(promise, after, [value], false);
   active(timeout);
 
   return promise;
@@ -430,36 +473,20 @@ function ontimeout(timer) {
     Reflect.apply(timer._onTimeout, timer, args);
 }
 
-function rearm(timer, start = TimerWrap.now()) {
+function rearm(timer, start) {
   // // Do not re-arm unenroll'd or closed timers.
-  if (timer._idleTimeout === -1) return;
+  if (timer._idleTimeout === -1)
+    return;
 
-  // If timer is unref'd (or was - it's permanently removed from the list.)
-  if (timer._handle && timer instanceof Timeout) {
-    timer._handle.start(timer._repeat);
-  } else {
-    timer._idleTimeout = timer._repeat;
-
-    const duration = TimerWrap.now() - start;
-    if (duration >= timer._repeat) {
-      // If callback duration >= timer._repeat,
-      // add 1 ms to avoid blocking eventloop
-      insert(timer, false, start + duration - timer._repeat + 1);
-    } else {
-      insert(timer, false, start);
-    }
-  }
+  timer._idleTimeout = timer._repeat;
+  insert(timer, timer[kRefed], start);
 }
 
 
 const clearTimeout = exports.clearTimeout = function clearTimeout(timer) {
   if (timer && timer._onTimeout) {
     timer._onTimeout = null;
-    if (timer instanceof Timeout) {
-      timer.close(); // for after === 0
-    } else {
-      unenroll(timer);
-    }
+    unenroll(timer);
   }
 };
 
@@ -490,7 +517,7 @@ exports.setInterval = function setInterval(callback, repeat, arg1, arg2, arg3) {
       break;
   }
 
-  const timeout = new Timeout(callback, repeat, args, true, false);
+  const timeout = new Timeout(callback, repeat, args, true);
   active(timeout);
 
   return timeout;
@@ -503,73 +530,25 @@ exports.clearInterval = function clearInterval(timer) {
   clearTimeout(timer);
 };
 
-function unrefdHandle(timer, now) {
-  try {
-    // Don't attempt to call the callback if it is not a function.
-    if (typeof timer._onTimeout === 'function') {
-      tryOnTimeout(timer, now);
-    }
-  } finally {
-    // Make sure we clean up if the callback is no longer a function
-    // even if the timer is an interval.
-    if (!timer._repeat || typeof timer._onTimeout !== 'function') {
-      timer.close();
-    }
-  }
-
-  return true;
-}
-
 
 Timeout.prototype.unref = function() {
-  if (this._handle) {
-    this._handle.unref();
-  } else if (typeof this._onTimeout === 'function') {
-    const now = TimerWrap.now();
-    if (!this._idleStart) this._idleStart = now;
-    var delay = this._idleStart + this._idleTimeout - now;
-    if (delay < 0) delay = 0;
-
-    // Prevent running cb again when unref() is called during the same cb
-    if (this._called && !this._repeat) {
-      unenroll(this);
-      return;
-    }
-
-    const handle = reuse(this);
-    if (handle !== null) {
-      handle._list = undefined;
-    }
-
-    this._handle = handle || new TimerWrap();
-    this._handle.owner = this;
-    this._handle.start(delay);
-    this._handle.unref();
+  if (this[kRefed]) {
+    this[kRefed] = false;
+    decRefCount();
   }
   return this;
 };
 
 Timeout.prototype.ref = function() {
-  if (this._handle)
-    this._handle.ref();
+  if (this[kRefed] === false) {
+    this[kRefed] = true;
+    incRefCount();
+  }
   return this;
 };
 
 Timeout.prototype.close = function() {
-  this._onTimeout = null;
-  if (this._handle) {
-    if (destroyHooksExist() &&
-        typeof this[async_id_symbol] === 'number' &&
-        !this._destroyed) {
-      emitDestroy(this[async_id_symbol]);
-      this._destroyed = true;
-    }
-
-    this._idleTimeout = -1;
-    this._handle.close();
-  } else {
-    unenroll(this);
-  }
+  clearTimeout(this);
   return this;
 };
 

src/timer_wrap.cc

@@ -62,7 +62,6 @@ class TimerWrap : public HandleWrap {
     env->SetProtoMethod(constructor, "hasRef", HandleWrap::HasRef);
 
     env->SetProtoMethod(constructor, "start", Start);
-    env->SetProtoMethod(constructor, "stop", Stop);
 
     target->Set(timerString, constructor->GetFunction());
 
@@ -125,32 +124,17 @@ class TimerWrap : public HandleWrap {
     args.GetReturnValue().Set(err);
   }
 
-  static void Stop(const FunctionCallbackInfo<Value>& args) {
-    TimerWrap* wrap;
-    ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
-
-    CHECK(HandleWrap::IsAlive(wrap));
-
-    int err = uv_timer_stop(&wrap->handle_);
-    args.GetReturnValue().Set(err);
-  }
-
   static void OnTimeout(uv_timer_t* handle) {
     TimerWrap* wrap = static_cast<TimerWrap*>(handle->data);
     Environment* env = wrap->env();
     HandleScope handle_scope(env->isolate());
     Context::Scope context_scope(env->context());
     Local<Value> ret;
-    Local<Value> args[1];
+    Local<Value> args[] = { env->GetNow() };
     do {
-      args[0] = env->GetNow();
       ret = wrap->MakeCallback(env->timers_callback_function(), 1, args)
                 .ToLocalChecked();
-    } while (ret->IsUndefined() &&
-             !env->tick_info()->has_thrown() &&
-             wrap->object()->Get(env->context(),
-                                 env->owner_string()).ToLocalChecked()
-                                                     ->IsUndefined());
+    } while (ret->IsUndefined() && !env->tick_info()->has_thrown());
   }
 
   static void Now(const FunctionCallbackInfo<Value>& args) {

test/async-hooks/test-timerwrap.setInterval.js

@@ -51,6 +51,6 @@ function onexit() {
   hooks.disable();
   hooks.sanityCheck('TIMERWRAP');
 
-  checkInvocations(t, { init: 1, before: 3, after: 3, destroy: 1 },
+  checkInvocations(t, { init: 1, before: 3, after: 3 },
                    't: when process exits');
 }

test/async-hooks/test-timerwrap.setTimeout.js

@@ -23,7 +23,6 @@ checkInvocations(t1, { init: 1 }, 't1: when first timer installed');
 function ontimeout() {
   checkInvocations(t1, { init: 1, before: 1 }, 't1: when first timer fired');
 
-  // install second timeout with same TIMEOUT to see timer wrap being reused
   setTimeout(onsecondTimeout, TIMEOUT);
   const as = hooks.activitiesOfTypes('TIMERWRAP');
   assert.strictEqual(as.length, 1);
@@ -31,35 +30,21 @@ function ontimeout() {
                    't1: when second timer installed');
 }
 
-// even though we install 3 timers we only have two timerwrap resources created
-// as one is reused for the two timers with the same timeout
-let t2;
-
 function onsecondTimeout() {
-  let as = hooks.activitiesOfTypes('TIMERWRAP');
+  const as = hooks.activitiesOfTypes('TIMERWRAP');
   assert.strictEqual(as.length, 1);
   checkInvocations(t1, { init: 1, before: 2, after: 1 },
                    't1: when second timer fired');
 
   // install third timeout with different TIMEOUT
   setTimeout(onthirdTimeout, TIMEOUT + 1);
-  as = hooks.activitiesOfTypes('TIMERWRAP');
-  assert.strictEqual(as.length, 2);
-  t2 = as[1];
-  assert.strictEqual(t2.type, 'TIMERWRAP');
-  assert.strictEqual(typeof t2.uid, 'number');
-  assert.strictEqual(typeof t2.triggerAsyncId, 'number');
   checkInvocations(t1, { init: 1, before: 2, after: 1 },
                    't1: when third timer installed');
-  checkInvocations(t2, { init: 1 },
-                   't2: when third timer installed');
 }
 
 function onthirdTimeout() {
-  checkInvocations(t1, { init: 1, before: 2, after: 2, destroy: 1 },
+  checkInvocations(t1, { init: 1, before: 3, after: 2 },
                    't1: when third timer fired');
-  checkInvocations(t2, { init: 1, before: 1 },
-                   't2: when third timer fired');
   tick(2);
 }
 
@@ -69,8 +54,6 @@ function onexit() {
   hooks.disable();
   hooks.sanityCheck('TIMERWRAP');
 
-  checkInvocations(t1, { init: 1, before: 2, after: 2, destroy: 1 },
+  checkInvocations(t1, { init: 1, before: 3, after: 3 },
                    't1: when process exits');
-  checkInvocations(t2, { init: 1, before: 1, after: 1, destroy: 1 },
-                   't2: when process exits');
 }

test/parallel/test-handle-wrap-isrefed.js

@@ -112,19 +112,23 @@ const dgram = require('dgram');
 
 // timers
 {
+  const { Timer } = process.binding('timer_wrap');
+  strictEqual(process._getActiveHandles().filter(
+    (handle) => (handle instanceof Timer)).length, 0);
   const timer = setTimeout(() => {}, 500);
-  timer.unref();
-  strictEqual(Object.getPrototypeOf(timer._handle).hasOwnProperty('hasRef'),
+  const handles = process._getActiveHandles().filter(
+    (handle) => (handle instanceof Timer));
+  strictEqual(handles.length, 1);
+  const handle = handles[0];
+  strictEqual(Object.getPrototypeOf(handle).hasOwnProperty('hasRef'),
               true, 'timer_wrap: hasRef() missing');
-  strictEqual(timer._handle.hasRef(),
+  strictEqual(handle.hasRef(), true);
+  timer.unref();
+  strictEqual(handle.hasRef(),
               false, 'timer_wrap: unref() ineffective');
   timer.ref();
-  strictEqual(timer._handle.hasRef(),
+  strictEqual(handle.hasRef(),
               true, 'timer_wrap: ref() ineffective');
-  timer._handle.close(common.mustCall(() =>
-    strictEqual(timer._handle.hasRef(),
-                false, 'timer_wrap: not unrefed on close')));
 }
 
-
 // see also test/pseudo-tty/test-handle-wrap-isrefed-tty.js

test/parallel/test-timers-next-tick.js

@@ -0,0 +1,15 @@
+'use strict';
+
+const common = require('../common');
+
+// This test documents an internal implementation detail of the Timers:
+// since timers of different durations are stored in separate lists,
+// a nextTick queue will clear after each list of timers. While this
+// behaviour is not documented it could be relied on by Node's users.
+
+setTimeout(common.mustCall(() => {
+  process.nextTick(() => { clearTimeout(t2); });
+}), 1);
+const t2 = setTimeout(common.mustNotCall(), 2);
+
+common.busyLoop(5);

test/parallel/test-timers-same-timeout-wrong-list-deleted.js

@@ -12,8 +12,6 @@
  */
 
 const common = require('../common');
-const assert = require('assert');
-const Timer = process.binding('timer_wrap').Timer;
 
 const TIMEOUT = common.platformTimeout(100);
 
@@ -30,41 +28,9 @@ const handle1 = setTimeout(common.mustCall(function() {
     // erroneously deleted. If we are able to cancel the timer successfully,
     // the bug is fixed.
     clearTimeout(handle2);
-
-    setImmediate(common.mustCall(function() {
-      setImmediate(common.mustCall(function() {
-        const activeTimers = getActiveTimers();
-
-        // Make sure our clearTimeout succeeded. One timer finished and
-        // the other was canceled, so none should be active.
-        assert.strictEqual(activeTimers.length, 0, 'Timers remain.');
-      }));
-    }));
   }), 1);
 
-  // Make sure our timers got added to the list.
-  const activeTimers = getActiveTimers();
-  const shortTimer = activeTimers.find(function(handle) {
-    return handle._list.msecs === 1;
-  });
-  const longTimers = activeTimers.filter(function(handle) {
-    return handle._list.msecs === TIMEOUT;
-  });
-
-  // Make sure our clearTimeout succeeded. One timer finished and
-  // the other was canceled, so none should be active.
-  assert.strictEqual(activeTimers.length, 3,
-                     'There should be 3 timers in the list.');
-  assert(shortTimer instanceof Timer, 'The shorter timer is not in the list.');
-  assert.strictEqual(longTimers.length, 2,
-                     'Both longer timers should be in the list.');
-
   // When this callback completes, `listOnTimeout` should now look at the
   // correct list and refrain from removing the new TIMEOUT list which
   // contains the reference to the newer timer.
 }), TIMEOUT);
-
-function getActiveTimers() {
-  const activeHandles = process._getActiveHandles();
-  return activeHandles.filter((handle) => handle instanceof Timer);
-}

test/parallel/test-timers-timeout-to-interval.js

@@ -8,5 +8,5 @@ const t = setTimeout(common.mustCall(() => {
   if (t._repeat) {
     clearInterval(t);
   }
-  t._repeat = true;
+  t._repeat = 1;
 }, 2), 1);

test/parallel/test-timers-unref-leak.js

@@ -1,14 +0,0 @@
-'use strict';
-const common = require('../common');
-
-const timeout = setTimeout(common.mustCall(), 10);
-timeout.unref();
-
-// Wrap `close` method to check if the handle was closed
-const close = timeout._handle.close;
-timeout._handle.close = common.mustCall(function() {
-  return close.apply(this, arguments);
-});
-
-// Just to keep process alive and let previous timer's handle die
-setTimeout(() => {}, 50);

test/parallel/test-timers-unref-reuse-no-exposed-list.js

@@ -1,14 +0,0 @@
-'use strict';
-
-require('../common');
-const assert = require('assert');
-
-const timer1 = setTimeout(() => {}, 1).unref();
-assert.strictEqual(timer1._handle._list, undefined,
-                   'timer1._handle._list should be undefined');
-
-// Check that everything works even if the handle was not re-used.
-setTimeout(() => {}, 1);
-const timer2 = setTimeout(() => {}, 1).unref();
-assert.strictEqual(timer2._handle._list, undefined,
-                   'timer2._handle._list should be undefined');