diff --git a/lib/timers.js b/lib/timers.js index a66ae5631da7..5acff0772604 100644 --- a/lib/timers.js +++ b/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. diff --git a/test/simple/test-timers-socket-timeout-removes-other-socket-unref-timer.js b/test/simple/test-timers-socket-timeout-removes-other-socket-unref-timer.js new file mode 100644 index 000000000000..3d7263c63480 --- /dev/null +++ b/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); +}); diff --git a/test/simple/test-timers-unref-remove-other-unref-timers-only-one-fires.js b/test/simple/test-timers-unref-remove-other-unref-timers-only-one-fires.js new file mode 100644 index 000000000000..5b4280f427b7 --- /dev/null +++ b/test/simple/test-timers-unref-remove-other-unref-timers-only-one-fires.js @@ -0,0 +1,56 @@ +// 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. + */ +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); diff --git a/test/simple/test-timers-unref-remove-other-unref-timers.js b/test/simple/test-timers-unref-remove-other-unref-timers.js new file mode 100644 index 000000000000..160e4824c97e --- /dev/null +++ b/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);