From 198eb9c5d6ac6a90dadb8c58396f9b35eaf6f5ce Mon Sep 17 00:00:00 2001
From: Anatoli Papirovski <apapirovski@mac.com>
Date: Wed, 11 Apr 2018 11:04:56 +0200
Subject: [PATCH] timers: reschedule interval even if it threw

To match browser behaviour, intervals should continue being
scheduled even if the user callback threw during execution.

PR-URL: https://github.com/nodejs/node/pull/20002
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 lib/timers.js                               | 15 +++++++--------
 test/async-hooks/test-timers.setInterval.js | 17 +++++++++++++----
 test/parallel/test-timers-interval-throw.js | 17 +++++++++++++++++
 3 files changed, 37 insertions(+), 12 deletions(-)
 create mode 100644 test/parallel/test-timers-interval-throw.js

diff --git a/lib/timers.js b/lib/timers.js
index 15700f5a1212ab..30bffb432ac26b 100644
--- 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
index a3434e134fab02..10b2bafb2a2598 100644
--- 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
new file mode 100644
index 00000000000000..876f0de55aa27e
--- /dev/null
+++ 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));