timers: fix a bug in error handling

apapirovski · MylesBorins · commit be3c8aad59e0 · 2018-05-15T12:17:53.000-04:00
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 <benjamingr@gmail.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
diff --git 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
@@ -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);
