nodejs · Fishrock123 · Aug 20, 2014 · Dec 18, 2014 · Aug 27, 2015 · thefourtheye
diff --git a/lib/timers.js b/lib/timers.js
@@ -477,47 +477,110 @@ exports.clearImmediate = function(immediate) {
 
 var unrefList, unrefTimer;
 
+function _makeTimerTimeout(timer) {
+  var domain = timer.domain;
+  var msecs = timer._idleTimeout;
+
+  L.remove(timer);
+
+  // Timer has been unenrolled by another timer that fired at the same time,
+  // so don't make it timeout.
+  if (msecs <= 0)
+    return;
+
+  if (!timer._onTimeout)
+    return;
+
+  if (domain) {
+    if (domain._disposed)
+      return;
+
+    domain.enter();
+  }
+
+  debug('unreftimer firing timeout');
+  timer._called = true;
+  _runOnTimeout(timer);
+
+  if (domain)
+    domain.exit();
+}
+
+function _runOnTimeout(timer) {
+  var threw = true;
+  try {
+    timer._onTimeout();
+    threw = false;
+  } finally {
+    if (threw) process.nextTick(unrefTimeout);
+  }
+}
 
 function unrefTimeout() {
   var now = Timer.now();
 
   debug('unrefTimer fired');
 
-  var diff, domain, first, threw;
-  while (first = L.peek(unrefList)) {
-    diff = now - first._idleStart;
+  var timeSinceLastActive;
+  var nextTimeoutTime;
+  var nextTimeoutDuration;
+  var minNextTimeoutTime = TIMEOUT_MAX;
+  var timersToTimeout = [];
+
+  // The actual timer fired and has not yet been rearmed,
+  // let's consider its next firing time is invalid for now.
+  // It may be set to a relevant time in the future once
+  // we scanned through the whole list of timeouts and if
+  // we find a timeout that needs to expire.
+  unrefTimer.when = -1;
 
-    if (diff < first._idleTimeout) {
-      diff = first._idleTimeout - diff;
-      unrefTimer.start(diff, 0);
-      unrefTimer.when = now + diff;
-      debug('unrefTimer rescheudling for later');
-      return;
+  // Iterate over the list of timeouts,
+  // call the onTimeout callback for those expired,
+  // and rearm the actual timer if the next timeout to expire
+  // will expire before the current actual timer.
+  var cur = unrefList._idlePrev;
+  while (cur !== unrefList) {
+    timeSinceLastActive = now - cur._idleStart;
+
+    if (timeSinceLastActive < cur._idleTimeout) {
+      // This timer hasn't expired yet, but check if its expiring time is
+      // earlier than the actual timer's expiring time
+
+      nextTimeoutDuration = cur._idleTimeout - timeSinceLastActive;
+      nextTimeoutTime = now + nextTimeoutDuration;
+      if (minNextTimeoutTime === TIMEOUT_MAX ||
+          (nextTimeoutTime < minNextTimeoutTime)) {
+        // We found a timeout that will expire earlier,
+        // store its next timeout time now so that we
+        // can rearm the actual timer accordingly when
+        // we scanned through the whole list.
+        minNextTimeoutTime = nextTimeoutTime;
+      }
+    } else {
+      // We found a timer that expired. Do not call its _onTimeout callback
+      // right now, as it could mutate any item of the list (including itself).
+      // Instead, add it to another list that will be processed once the list
+      // of current timers has been fully traversed.
+      timersToTimeout.push(cur);
     }
 
-    L.remove(first);
+    cur = cur._idlePrev;
+  }
 
-    domain = first.domain;
+  var nbTimersToTimeout = timersToTimeout.length;
+  for (var timerIdx = 0; timerIdx < nbTimersToTimeout; ++timerIdx)
+    _makeTimerTimeout(timersToTimeout[timerIdx]);
 
-    if (!first._onTimeout) continue;
-    if (domain && domain._disposed) continue;
 
-    try {
-      if (domain) domain.enter();
-      threw = true;
-      debug('unreftimer firing timeout');
-      first._called = true;
-      first._onTimeout();
-      threw = false;
-      if (domain)
-        domain.exit();
-    } finally {
-      if (threw) process.nextTick(unrefTimeout);
-    }
+  // Rearm the actual timer with the timeout delay
+  // of the earliest timeout found.
+  if (minNextTimeoutTime !== TIMEOUT_MAX) {
+    unrefTimer.start(minNextTimeoutTime - now, 0);
+    unrefTimer.when = minNextTimeoutTime;
+    debug('unrefTimer rescheduled');
+  } else if (L.isEmpty(unrefList)) {
+    debug('unrefList is empty');
   }
-
-  debug('unrefList is empty');
-  unrefTimer.when = -1;
 }
 
 
@@ -543,38 +606,14 @@ exports._unrefActive = function(item) {
   var now = Timer.now();
   item._idleStart = now;
 
-  if (L.isEmpty(unrefList)) {
-    debug('unrefList empty');
-    L.append(unrefList, item);
+  var when = now + msecs;
 
+  // If the actual timer is set to fire too late, or not set to fire at all,
+  // we need to make it fire earlier
+  if (unrefTimer.when === -1 || unrefTimer.when > when) {
     unrefTimer.start(msecs, 0);
-    unrefTimer.when = now + msecs;
+    unrefTimer.when = when;
     debug('unrefTimer scheduled');
-    return;
-  }
-
-  var when = now + msecs;
-
-  debug('unrefList find where we can insert');
-
-  var cur, them;
-
-  for (cur = unrefList._idlePrev; cur != unrefList; cur = cur._idlePrev) {
-    them = cur._idleStart + cur._idleTimeout;
-
-    if (when < them) {
-      debug('unrefList inserting into middle of list');
-
-      L.append(cur, item);
-
-      if (unrefTimer.when > when) {
-        debug('unrefTimer is scheduled to fire too late, reschedule');
-        unrefTimer.start(msecs, 0);
-        unrefTimer.when = when;
-      }
-
-      return;
-    }
   }
 
   debug('unrefList append to end');

diff --git a/test/parallel/test-timers-socket-timeout-removes-other-socket-unref-timer.js b/test/parallel/test-timers-socket-timeout-removes-other-socket-unref-timer.js
@@ -0,0 +1,50 @@
+'use strict';
+
+/*
+ * This test is a regression test for joyent/node#8897.
+ */
+
+const common = require('../common');
+const assert = require('assert');
+const net = require('net');
+
+const clients = [];
+
+const server = net.createServer(function onClient(client) {
+  clients.push(client);
+
+  if (clients.length === 2) {
+    /*
+     * Enroll two timers, and make the one supposed to fire first
+     * unenroll the other one supposed to fire later. This mutates
+     * the list of unref timers when traversing it, and exposes the
+     * original issue in joyent/node#8897.
+     */
+    clients[0].setTimeout(1, function onTimeout() {
+      clients[1].setTimeout(0);
+      clients[0].end();
+      clients[1].end();
+    });
+
+    // Use a delay that is higher than the lowest timer resolution accross all
+    // supported platforms, so that the two timers don't fire at the same time.
+    clients[1].setTimeout(50);
+  }
+});
+
+server.listen(common.PORT, common.localhostIPv4, function() {
+  var nbClientsEnded = 0;
+
+  function addEndedClient(client) {
+    ++nbClientsEnded;
+    if (nbClientsEnded === 2) {
+      server.close();
+    }
+  };
+
+  const client1 = net.connect({ port: common.PORT });
+  client1.on('end', addEndedClient);
+
+  const client2 = net.connect({ port: common.PORT });
+  client2.on('end', addEndedClient);
+});
diff --git a/test/parallel/test-timers-unref-active-unenrolled-disposed.js b/test/parallel/test-timers-unref-active-unenrolled-disposed.js
@@ -0,0 +1,46 @@
+'use strict';
+
+// https://github.com/nodejs/node/pull/2540/files#r38231197
+
+const common = require('../common');
+const timers = require('timers');
+const assert = require('assert');
+const domain = require('domain');
+
+// Crazy stuff to keep the process open,
+// then close it when we are actually done.
+const TEST_DURATION = common.platformTimeout(100);
+const keepOpen = setTimeout(function() {
+  throw new Error('Test timed out. keepOpen was not canceled.');
+}, TEST_DURATION);
+
+const endTest = makeTimer(2);
+
+const someTimer = makeTimer(1);
+someTimer.domain = domain.create();
+someTimer.domain.dispose();
+someTimer._onTimeout = function() {
+  throw new Error('someTimer was not supposed to fire!');
+};
+
+endTest._onTimeout = common.mustCall(function() {
+  assert.strictEqual(someTimer._idlePrev, null);
+  assert.strictEqual(someTimer._idleNext, null);
+  clearTimeout(keepOpen);
+});
+
+const cancelsTimer = makeTimer(1);
+cancelsTimer._onTimeout = common.mustCall(function() {
+  someTimer._idleTimeout = 0;
+});
+
+timers._unrefActive(cancelsTimer);
+timers._unrefActive(someTimer);
+timers._unrefActive(endTest);
+
+function makeTimer(msecs) {
+  const timer = {};
+  timers.unenroll(timer);
+  timers.enroll(timer, msecs);
+  return timer;
+}
diff --git a/test/parallel/test-timers-unref-active.js b/test/parallel/test-timers-unref-active.js
@@ -0,0 +1,51 @@
+'use strict';
+
+/*
+ * This test is aimed at making sure that unref timers queued with
+ * timers._unrefActive work correctly.
+ *
+ * Basically, it queues one timer in the unref queue, and then queues
+ * it again each time its timeout callback is fired until the callback
+ * has been called ten times.
+ *
+ * At that point, it unenrolls the unref timer so that its timeout callback
+ * is not fired ever again.
+ *
+ * Finally, a ref timeout is used with a delay large enough to make sure that
+ * all 10 timeouts had the time to expire.
+ */
+
+const common = require('../common');
+const timers = require('timers');
+const assert = require('assert');
+
+var someObject = {};
+var nbTimeouts = 0;
+
+/*
+ * libuv 0.10.x uses GetTickCount on Windows to implement timers, which uses
+ * system's timers whose resolution is between 10 and 16ms. See
+ * http://msdn.microsoft.com/en-us/library/windows/desktop/ms724408.aspx
+ * for more information. That's the lowest resolution for timers across all
+ * supported platforms. We're using it as the lowest common denominator,
+ * and thus expect 5 timers to be able to fire in under 100 ms.
+ */
+const N = 5;
+const TEST_DURATION = 100;
+
+timers.unenroll(someObject);
+timers.enroll(someObject, 1);
+
+someObject._onTimeout = function _onTimeout() {
+  ++nbTimeouts;
+
+  if (nbTimeouts === N) timers.unenroll(someObject);
+
+  timers._unrefActive(someObject);
+};
+
+timers._unrefActive(someObject);
+
+setTimeout(function() {
+  assert.equal(nbTimeouts, N);
+}, TEST_DURATION);
diff --git a/test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js b/test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js
@@ -0,0 +1,41 @@
+'use strict';
+
+/*
+ * The goal of this test is to make sure that, after the regression introduced
+ * by 934bfe23a16556d05bfb1844ef4d53e8c9887c3d, the fix preserves the following
+ * behavior of unref timers: if two timers are scheduled to fire at the same
+ * time, if one unenrolls the other one in its _onTimeout callback, the other
+ * one will *not* fire.
+ *
+ * This behavior is a private implementation detail and should not be
+ * considered public interface.
+ */
+const common = require('../common');
+const timers = require('timers');
+const assert = require('assert');
+
+var nbTimersFired = 0;
+
+const foo = {
+  _onTimeout: function() {
+    ++nbTimersFired;
+    timers.unenroll(bar);
+  }
+};
+
+const bar = {
+  _onTimeout: function() {
+    ++nbTimersFired;
+    timers.unenroll(foo);
+  }
+};
+
+timers.enroll(bar, 1);
+timers._unrefActive(bar);
+
+timers.enroll(foo, 1);
+timers._unrefActive(foo);
+
+setTimeout(function() {
+  assert.notEqual(nbTimersFired, 2);
+}, 20);