From 03bb7d76b033027bf7b53558a3834f8d3b01cd2f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 13 Apr 2018 10:37:28 +0200 Subject: [PATCH 1/2] timers: call destroy on interval error When an interval callback throws an error, the destroy hook is never called due to a faulty if condition. --- lib/timers.js | 2 +- test/async-hooks/test-timers.setInterval.js | 38 +++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 test/async-hooks/test-timers.setInterval.js diff --git a/lib/timers.js b/lib/timers.js index 88fd31ad4ccdb7..1786fb08b33c06 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -292,7 +292,7 @@ function tryOnTimeout(timer, start) { if (timerAsyncId !== null) { if (!threw) emitAfter(timerAsyncId); - if (!timer._repeat && destroyHooksExist() && + if ((threw || !timer._repeat) && destroyHooksExist() && !timer._destroyed) { emitDestroy(timerAsyncId); timer._destroyed = true; diff --git a/test/async-hooks/test-timers.setInterval.js b/test/async-hooks/test-timers.setInterval.js new file mode 100644 index 00000000000000..472ec089c23bb8 --- /dev/null +++ b/test/async-hooks/test-timers.setInterval.js @@ -0,0 +1,38 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const initHooks = require('./init-hooks'); +const { checkInvocations } = require('./hook-checks'); +const TIMEOUT = common.platformTimeout(100); + +const hooks = initHooks(); +hooks.enable(); + +// install first timeout +setInterval(common.mustCall(ontimeout), TIMEOUT); +const as = hooks.activitiesOfTypes('Timeout'); +assert.strictEqual(as.length, 1); +const t1 = as[0]; +assert.strictEqual(t1.type, 'Timeout'); +assert.strictEqual(typeof t1.uid, 'number'); +assert.strictEqual(typeof t1.triggerAsyncId, 'number'); +checkInvocations(t1, { init: 1 }, 't1: when timer installed'); + +function ontimeout() { + checkInvocations(t1, { init: 1, before: 1 }, 't1: when first timer fired'); + + throw new Error('setInterval Error'); +} + +process.on('uncaughtException', common.mustCall((err) => { + assert(err.message, 'setInterval Error'); +})); + +process.on('exit', () => { + hooks.disable(); + hooks.sanityCheck('Timeout'); + + checkInvocations(t1, { init: 1, before: 1, after: 1, destroy: 1 }, + 't1: when process exits'); +}); From 887af5a22337324ba8d03e1fb00ec67cb5540ce4 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 13 Apr 2018 11:41:07 +0200 Subject: [PATCH 2/2] fixup: adjust test based on feedback --- test/async-hooks/test-timers.setInterval.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/async-hooks/test-timers.setInterval.js b/test/async-hooks/test-timers.setInterval.js index 472ec089c23bb8..a3434e134fab02 100644 --- a/test/async-hooks/test-timers.setInterval.js +++ b/test/async-hooks/test-timers.setInterval.js @@ -9,7 +9,6 @@ const TIMEOUT = common.platformTimeout(100); const hooks = initHooks(); hooks.enable(); -// install first timeout setInterval(common.mustCall(ontimeout), TIMEOUT); const as = hooks.activitiesOfTypes('Timeout'); assert.strictEqual(as.length, 1); @@ -25,8 +24,8 @@ function ontimeout() { throw new Error('setInterval Error'); } -process.on('uncaughtException', common.mustCall((err) => { - assert(err.message, 'setInterval Error'); +process.once('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, 'setInterval Error'); })); process.on('exit', () => {