timers: fix a bug in error handling

When a timeout within a list of timeouts (that consists of only
that specific timeout) throws during its execution, it's possible
for the TimerWrap handle to become shared between both that list
and an unref'd timeout created in the future. This fixes the bug
by extending error handling in timeout execution to check for
whether the current list is empty and if so do cleanup on it.

PR-URL: #20497
Fixes: #19970
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>

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

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