nodejs
diff --git a/‎lib/timers.js‎
Lines changed: 45 additions & 36 deletions b/‎lib/timers.js‎
Lines changed: 45 additions & 36 deletions
diff --git a/‎test/parallel/test-timers-socket-timeout-removes-other-socket-unref-timer.js‎
Lines changed: 50 additions & 0 deletions b/‎test/parallel/test-timers-socket-timeout-removes-other-socket-unref-timer.js‎
Lines changed: 50 additions & 0 deletions
diff --git a/‎test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js‎
Lines changed: 41 additions & 0 deletions b/‎test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js‎
Lines changed: 41 additions & 0 deletions
diff --git a/‎test/parallel/test-timers-unref-remove-other-unref-timers.js‎
Lines changed: 33 additions & 0 deletions b/‎test/parallel/test-timers-unref-remove-other-unref-timers.js‎
Lines changed: 33 additions & 0 deletions
@@ -477,6 +477,39 @@ exports.clearImmediate = function(immediate) {
 
 var unrefList, unrefTimer;
 
+function _makeTimerTimeout(timer) {
+  var domain = timer.domain;
+  var msecs = timer._idleTimeout;
+
+  // Timer has been unenrolled by another timer that fired at the same time,
+  // so don't make it timeout.
+  if (!msecs || msecs < 0)
+    return;
+
+  if (!timer._onTimeout)
+    return;
+
+  if (domain && domain._disposed)
+    return;
+
+  try {
+    var threw = true;
+
+    if (domain) domain.enter();
+
+    debug('unreftimer firing timeout');
+    L.remove(timer);
+    timer._called = true;
+    timer._onTimeout();
+
+    threw = false;
+
+    if (domain)
+      domain.exit();
+  } finally {
+    if (threw) process.nextTick(unrefTimeout);
+  }
+}
 
 function unrefTimeout() {
   var now = Timer.now();
@@ -487,7 +520,7 @@ function unrefTimeout() {
   var nextTimeoutTime;
   var nextTimeoutDuration;
   var minNextTimeoutTime;
-  var itemToDelete;
+  var timersToTimeout = [];
 
   // The actual timer fired and has not yet been rearmed,
   // let's consider its next firing time is invalid for now.
@@ -518,45 +551,21 @@ function unrefTimeout() {
         // we scanned through the whole list.
         minNextTimeoutTime = nextTimeoutTime;
       }
-
-      // This timer hasn't expired yet, skipping
-      cur = cur._idlePrev;
-      continue;
+    } 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);
     }
 
-    // We found a timer that expired
-    var domain = cur.domain;
-
-    if (!cur._onTimeout) continue;
-
-    if (domain && domain._disposed)
-      continue;
-
-    try {
-      var threw = true;
-
-      if (domain) domain.enter();
-
-      itemToDelete = cur;
-      // Move to the previous item before calling the _onTimeout callback,
-      // as it can mutate the list.
-      cur = cur._idlePrev;
-
-      // Remove the timeout from the list because it expired.
-      L.remove(itemToDelete);
-
-      debug('unreftimer firing timeout');
-      itemToDelete._called = true;
-      itemToDelete._onTimeout();
+    cur = cur._idlePrev;
+  }
 
-      threw = false;
+  var nbTimersToTimeout = timersToTimeout.length;
+  for (var timerIdx = 0; timerIdx < nbTimersToTimeout; ++timerIdx)
+    _makeTimerTimeout(timersToTimeout[timerIdx]);
 
-      if (domain)
-        domain.exit();
-    } finally {
-      if (threw) process.nextTick(unrefTimeout);
-    }
-  }
 
   // Rearm the actual timer with the timeout delay
   // of the earliest timeout found.
 
@@ -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);
+});
@@ -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);
@@ -0,0 +1,33 @@
+'use strict';
+
+/*
+ * This test is a regression test for joyent/node#8897.
+ *
+ * It tests some private implementation details that should not be
+ * considered public interface.
+ */
+const common = require('../common');
+const assert = require('assert');
+const timers = require('timers');
+
+const foo = {
+  _onTimeout: assert.fail
+};
+
+const bar = {
+  _onTimeout: common.mustCall(function() {
+    timers.unenroll(foo);
+  })
+};
+
+// We use timers with expiration times that are sufficiently apart to make
+// sure that they're not fired at the same time on platforms where the timer
+// resolution is a bit coarse (e.g Windows with a default resolution of ~15ms).
+timers.enroll(bar, 1);
+timers._unrefActive(bar);
+
+timers.enroll(foo, 50);
+timers._unrefActive(foo);
+
+// Keep the process open.
+setTimeout(function() {}, 100);