diff --git a/lib/timers.js b/lib/timers.js index 7ae1879b14..82ac156816 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -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. 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 new file mode 100644 index 0000000000..081688cfa5 --- /dev/null +++ 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-remove-other-unref-timers-only-one-fires.js b/test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js new file mode 100644 index 0000000000..aead4a4e7d --- /dev/null +++ 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); diff --git a/test/parallel/test-timers-unref-remove-other-unref-timers.js b/test/parallel/test-timers-unref-remove-other-unref-timers.js new file mode 100644 index 0000000000..f727d5f86f --- /dev/null +++ b/test/parallel/test-timers-unref-remove-other-unref-timers.js @@ -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);