diff --git a/lib/timers.js b/lib/timers.js index f3c3c6308433eb..e6655c2f527349 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -217,6 +217,17 @@ function TimersList(msecs, unrefed) { this.nextTick = false; } +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]; + } +} + function listOnTimeout() { var list = this._list; var msecs = list.msecs; @@ -288,14 +299,7 @@ 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). @@ -329,24 +333,34 @@ function tryOnTimeout(timer, list) { } } - if (!threw) return; + if (threw) { + const { msecs } = list; + + if (L.isEmpty(list)) { + deleteTimersList(list, msecs); - // 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; + 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);