timers: don't mutate unref list while iterating it

lib/timers.js

@@ -404,6 +404,38 @@ 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._onTimeout();
+
+    threw = false;
+
+    if (domain)
+      domain.exit();
+  } finally {
+    if (threw) process.nextTick(unrefTimeout);
+  }
+}
 
 function unrefTimeout() {
   var now = Timer.now();
@@ -414,7 +446,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.
@@ -445,44 +477,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._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.

test/simple/test-timers-socket-timeout-removes-other-socket-unref-timer.js

@@ -0,0 +1,69 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+/*
+ * This test is a regression test for joyent/node#8897.
+ */
+
+var common = require('../common');
+var assert = require('assert');
+var net = require('net');
+
+var clients = [];
+
+var 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, '127.0.0.1', function() {
+  var nbClientsEnded = 0;
+
+  function addEndedClient(client) {
+    ++nbClientsEnded;
+    if (nbClientsEnded === 2) {
+      server.close();
+    }
+  }
+
+  var client1 = net.connect({ port: common.PORT })
+  client1.on('end', addEndedClient);
+
+  var client2 = net.connect({ port: common.PORT });
+  client2.on('end', addEndedClient);
+});

test/simple/test-timers-unref-remove-other-unref-timers-only-one-fires.js

@@ -0,0 +1,59 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+/*
+ * 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.
+ */
+var timers = require('timers');
+var assert = require('assert');
+
+var nbTimersFired = 0;
+
+var foo = new function() {
+  this._onTimeout = function() {
+    ++nbTimersFired;
+    timers.unenroll(bar);
+  };
+}();
+
+var bar = new function() {
+  this._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);

test/simple/test-timers-unref-remove-other-unref-timers.js

@@ -0,0 +1,49 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+/*
+ * This test is a regression test for joyent/node#8897.
+ *
+ * It tests some private implementation details that should not be
+ * considered public interface.
+ */
+var timers = require('timers');
+
+var foo = new function() {
+  this._onTimeout = function() {};
+}();
+
+var bar = new function() {
+  this._onTimeout = 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);
+
+setTimeout(function() {}, 100);