[v9.x bugfix] timers: fix a bug in error handling

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;
     }
   }
 }

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);