diff --git a/lib/timers.js b/lib/timers.js index 4d3f655a1a1271..f956e418632597 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -217,6 +217,17 @@ function TimersList(msecs, unrefed) { timer.start(msecs); } +function deleteTimersList(list, msecs) { + // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and + // recreated since the reference to `list` was created. Make sure they're + // the same instance of the list before destroying. + if (list._unrefed === true && list === unrefedLists[msecs]) { + delete unrefedLists[msecs]; + } else if (list === refedLists[msecs]) { + delete refedLists[msecs]; + } +} + // adds listOnTimeout to the C++ object prototype, as // V8 would not inline it otherwise. TimerWrap.prototype[kOnTimeout] = function listOnTimeout() { @@ -273,14 +284,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() { debug('%d list empty', msecs); assert(L.isEmpty(list)); - // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and - // recreated since the reference to `list` was created. Make sure they're - // the same instance of the list before destroying. - if (list._unrefed === true && list === unrefedLists[msecs]) { - delete unrefedLists[msecs]; - } else if (list === refedLists[msecs]) { - delete refedLists[msecs]; - } + deleteTimersList(list, msecs); // Do not close the underlying handle if its ownership has changed // (e.g it was unrefed in its callback). @@ -315,22 +319,32 @@ function tryOnTimeout(timer, list) { } if (threw) { - // Postpone all later list events to next tick. We need to do this - // so that the events are called in the order they were created. - const lists = list._unrefed === true ? unrefedLists : refedLists; - for (var key in lists) { - if (key > list.msecs) { - lists[key].nextTick = true; + const { msecs } = list; + + if (L.isEmpty(list)) { + deleteTimersList(list, msecs); + + if (!list._timer.owner) + list._timer.close(); + } else { + // Postpone all later list events to next tick. We need to do this + // so that the events are called in the order they were created. + const lists = list._unrefed === true ? unrefedLists : refedLists; + for (var key in lists) { + if (key > msecs) { + lists[key].nextTick = true; + } } + + // We need to continue processing after domain error handling + // is complete, but not by using whatever domain was left over + // when the timeout threw its exception. + const domain = process.domain; + process.domain = null; + // If we threw, we need to process the rest of the list in nextTick. + process.nextTick(listOnTimeoutNT, list); + process.domain = domain; } - // We need to continue processing after domain error handling - // is complete, but not by using whatever domain was left over - // when the timeout threw its exception. - const domain = process.domain; - process.domain = null; - // If we threw, we need to process the rest of the list in nextTick. - process.nextTick(listOnTimeoutNT, list); - process.domain = domain; } } } diff --git a/test/parallel/test-timers-throw-reschedule.js b/test/parallel/test-timers-throw-reschedule.js new file mode 100644 index 00000000000000..b804b6effa9c69 --- /dev/null +++ b/test/parallel/test-timers-throw-reschedule.js @@ -0,0 +1,27 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +// This test checks that throwing inside a setTimeout where that Timeout +// instance is the only one within its list of timeouts, doesn't cause +// an issue with an unref timeout scheduled in the error handler. +// Refs: https://github.com/nodejs/node/issues/19970 + +const timeout = common.platformTimeout(50); + +const timer = setTimeout(common.mustNotCall(), 2 ** 31 - 1); + +process.once('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, 'setTimeout Error'); + + const now = Date.now(); + setTimeout(common.mustCall(() => { + assert(Date.now() - now >= timeout); + clearTimeout(timer); + }), timeout).unref(); +})); + +setTimeout(() => { + throw new Error('setTimeout Error'); +}, timeout);