timers: reschedule interval even if it threw

To match browser behaviour, intervals should continue being scheduled even if the user callback threw during execution.
nodejs · Apr 28, 2018 · 350b126 · 350b126
1 parent e797d5b
commit 350b126
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 12 deletions.
diff --git a/lib/timers.js b/lib/timers.js
@@ -285,15 +285,18 @@ function tryOnTimeout(timer, start) {
   var threw = true;
   if (timerAsyncId !== null)
     emitBefore(timerAsyncId, timer[trigger_async_id_symbol]);
+  if (start === undefined && timer._repeat)
+    start = TimerWrap.now();
   try {
-    ontimeout(timer, start);
+    ontimeout(timer);
     threw = false;
   } finally {
     if (timerAsyncId !== null) {
       if (!threw)
         emitAfter(timerAsyncId);
-      if ((threw || !timer._repeat) && destroyHooksExist() &&
-          !timer._destroyed) {
+      if (timer._repeat) {
+        rearm(timer, start);
+      } else if (destroyHooksExist() && !timer._destroyed) {
         emitDestroy(timerAsyncId);
         timer._destroyed = true;
       }
@@ -417,18 +420,14 @@ setTimeout[internalUtil.promisify.custom] = function(after, value) {
 exports.setTimeout = setTimeout;
 
 
-function ontimeout(timer, start) {
+function ontimeout(timer) {
   const args = timer._timerArgs;
   if (typeof timer._onTimeout !== 'function')
     return promiseResolve(timer._onTimeout, args[0]);
-  if (start === undefined && timer._repeat)
-    start = TimerWrap.now();
   if (!args)
     timer._onTimeout();
   else
     Reflect.apply(timer._onTimeout, timer, args);
-  if (timer._repeat)
-    rearm(timer, start);
 }
 
 function rearm(timer, start = TimerWrap.now()) {

diff --git a/test/async-hooks/test-timers.setInterval.js b/test/async-hooks/test-timers.setInterval.js
@@ -9,7 +9,7 @@ const TIMEOUT = common.platformTimeout(100);
 const hooks = initHooks();
 hooks.enable();
 
-setInterval(common.mustCall(ontimeout), TIMEOUT);
+const interval = setInterval(common.mustCall(ontimeout, 2), TIMEOUT);
 const as = hooks.activitiesOfTypes('Timeout');
 assert.strictEqual(as.length, 1);
 const t1 = as[0];
@@ -18,9 +18,18 @@ assert.strictEqual(typeof t1.uid, 'number');
 assert.strictEqual(typeof t1.triggerAsyncId, 'number');
 checkInvocations(t1, { init: 1 }, 't1: when timer installed');
 
+let iter = 0;
 function ontimeout() {
-  checkInvocations(t1, { init: 1, before: 1 }, 't1: when first timer fired');
-
+  if (iter === 0) {
+    checkInvocations(t1, { init: 1, before: 1 }, 't1: when first timer fired');
+  } else {
+    checkInvocations(t1, { init: 1, before: 2, after: 1 },
+                     't1: when first interval fired again');
+    clearInterval(interval);
+    return;
+  }
+
+  iter++;
   throw new Error('setInterval Error');
 }
 
@@ -32,6 +41,6 @@ process.on('exit', () => {
   hooks.disable();
   hooks.sanityCheck('Timeout');
 
-  checkInvocations(t1, { init: 1, before: 1, after: 1, destroy: 1 },
+  checkInvocations(t1, { init: 1, before: 2, after: 2, destroy: 1 },
                    't1: when process exits');
 });
diff --git a/test/parallel/test-timers-interval-throw.js b/test/parallel/test-timers-interval-throw.js
@@ -0,0 +1,17 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+// To match browser behaviour, interval should continue
+// being rescheduled even if it throws.
+
+let count = 2;
+const interval = setInterval(() => { throw new Error('IntervalError'); }, 1);
+
+process.on('uncaughtException', common.mustCall((err) => {
+  assert.strictEqual(err.message, 'IntervalError');
+  if (--count === 0) {
+    clearInterval(interval);
+  }
+}, 2));