From ea3f061bc27151286de5795536a9b5b626a79025 Mon Sep 17 00:00:00 2001 From: Shigeki Ohtsu Date: Mon, 20 Feb 2017 14:31:15 +0900 Subject: [PATCH] timers: fix not to close reused timer handle The timer handle is reused when it is unrefed in order to avoid new callback in beforeExit(#3407). If it is unrefed within a setInterval callback, the reused timer handle is closed so that setInterval no longer keep working. This fix does not close the handle in case of setInterval. PR-URL: https://github.com/nodejs/node/pull/11646 Reviewed-By: Julien Gilli Reviewed-By: Jeremiah Senkpiel Reviewed-By: James M Snell --- lib/timers.js | 8 ++- .../test-timers-unrefed-in-callback.js | 61 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-timers-unrefed-in-callback.js diff --git a/lib/timers.js b/lib/timers.js index 0784f7f1e11247..94567f7990b994 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -222,7 +222,6 @@ function listOnTimeout() { // As such, we can remove the list and clean up the TimerWrap C++ handle. debug('%d list empty', msecs); assert(L.isEmpty(list)); - this.close(); // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and // recreated since the reference to `list` was created. Make sure they're @@ -232,6 +231,13 @@ function listOnTimeout() { } else if (list === refedLists[msecs]) { delete refedLists[msecs]; } + + // Do not close the underlying handle if its ownership has changed + // (e.g it was unrefed in its callback). + if (this.owner) + return; + + this.close(); } diff --git a/test/parallel/test-timers-unrefed-in-callback.js b/test/parallel/test-timers-unrefed-in-callback.js new file mode 100644 index 00000000000000..c6fcb22789955a --- /dev/null +++ b/test/parallel/test-timers-unrefed-in-callback.js @@ -0,0 +1,61 @@ +'use strict'; +// Checks that setInterval timers keep running even when they're +// unrefed within their callback. + +require('../common'); +const assert = require('assert'); +const net = require('net'); + +let counter1 = 0; +let counter2 = 0; + +// Test1 checks that clearInterval works as expected for a timer +// unrefed within its callback: it removes the timer and its callback +// is not called anymore. Note that the only reason why this test is +// robust is that: +// 1. the repeated timer it creates has a delay of 1ms +// 2. when this test is completed, another test starts that creates a +// new repeated timer with the same delay (1ms) +// 3. because of the way timers are implemented in libuv, if two +// repeated timers A and B are created in that order with the same +// delay, it is guaranteed that the first occurrence of timer A +// will fire before the first occurrence of timer B +// 4. as a result, when the timer created by Test2 fired 11 times, if +// the timer created by Test1 hadn't been removed by clearInterval, +// it would have fired 11 more times, and the assertion in the +// process'exit event handler would fail. +function Test1() { + // server only for maintaining event loop + const server = net.createServer().listen(0); + + const timer1 = setInterval(() => { + timer1.unref(); + if (counter1++ === 3) { + clearInterval(timer1); + server.close(() => { + Test2(); + }); + } + }, 1); +} + + +// Test2 checks setInterval continues even if it is unrefed within +// timer callback. counter2 continues to be incremented more than 11 +// until server close completed. +function Test2() { + // server only for maintaining event loop + const server = net.createServer().listen(0); + + const timer2 = setInterval(() => { + timer2.unref(); + if (counter2++ === 3) + server.close(); + }, 1); +} + +process.on('exit', () => { + assert.strictEqual(counter1, 4); +}); + +Test1();