From 005a242e8e9d93c2f019a62898a47e566725afa8 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 5 Feb 2018 09:47:01 -0500 Subject: [PATCH 1/4] timers: remove unused variable A recent commit removed the usage of the second argument of tryOnTimeout but left the definition in place. Remove it. --- lib/timers.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/timers.js b/lib/timers.js index 79fc432dcdf3d7..1c9249bb5e81a0 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -260,7 +260,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) { continue; } - tryOnTimeout(timer, list); + tryOnTimeout(timer); } // If `L.peek(list)` returned nothing, the list was either empty or we have @@ -289,7 +289,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) { // An optimization so that the try/finally only de-optimizes (since at least v8 // 4.7) what is in this smaller function. -function tryOnTimeout(timer, list) { +function tryOnTimeout(timer) { timer._called = true; const timerAsyncId = (typeof timer[async_id_symbol] === 'number') ? timer[async_id_symbol] : null; From fe9c54c48f49bc2fb5d0cd7bed71e8c4bfe95ade Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 5 Feb 2018 09:50:18 -0500 Subject: [PATCH 2/4] timers: be more defensive with intervals It's possible for user-code to flip an existing timeout to be an interval during its execution, in which case the current code would crash due to start being undefined. Fix this by providing a default start value within rearm. --- lib/timers.js | 3 +-- test/parallel/test-timers-timeout-to-interval.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-timers-timeout-to-interval.js diff --git a/lib/timers.js b/lib/timers.js index 1c9249bb5e81a0..8ddd5f894fef33 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -442,8 +442,7 @@ function ontimeout(timer, start) { rearm(timer, start); } - -function rearm(timer, start) { +function rearm(timer, start = TimerWrap.now()) { // // Do not re-arm unenroll'd or closed timers. if (timer._idleTimeout === -1) return; diff --git a/test/parallel/test-timers-timeout-to-interval.js b/test/parallel/test-timers-timeout-to-interval.js new file mode 100644 index 00000000000000..6952f2231a7e18 --- /dev/null +++ b/test/parallel/test-timers-timeout-to-interval.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../common'); + +// This isn't officially supported but nonetheless is something that is +// currently possible and as such it shouldn't cause the process to crash + +const t = setTimeout(common.mustCall(() => { + if (t._repeat) { + clearInterval(t); + } + t._repeat = true; +}, 2), 1); From 7e70d3aa0ce9c3a78bcf97c5347658ebfc9f0e33 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 5 Feb 2018 11:19:11 -0500 Subject: [PATCH 3/4] timers: async track unref timers When async hooks integration for Timers was introduced, it was not included in the code for unref'd or subsequently ref'd timers which means those timers only have Timerwrap hooks. --- lib/timers.js | 6 +-- test/async-hooks/test-timers.setTimeout.js | 61 ++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 test/async-hooks/test-timers.setTimeout.js diff --git a/lib/timers.js b/lib/timers.js index 8ddd5f894fef33..1ea4c603719c6f 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -289,7 +289,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) { // An optimization so that the try/finally only de-optimizes (since at least v8 // 4.7) what is in this smaller function. -function tryOnTimeout(timer) { +function tryOnTimeout(timer, start) { timer._called = true; const timerAsyncId = (typeof timer[async_id_symbol] === 'number') ? timer[async_id_symbol] : null; @@ -297,7 +297,7 @@ function tryOnTimeout(timer) { if (timerAsyncId !== null) emitBefore(timerAsyncId, timer[trigger_async_id_symbol]); try { - ontimeout(timer); + ontimeout(timer, start); threw = false; } finally { if (timerAsyncId !== null) { @@ -520,7 +520,7 @@ function unrefdHandle(now) { try { // Don't attempt to call the callback if it is not a function. if (typeof this.owner._onTimeout === 'function') { - ontimeout(this.owner, now); + tryOnTimeout(this.owner, now); } } finally { // Make sure we clean up if the callback is no longer a function diff --git a/test/async-hooks/test-timers.setTimeout.js b/test/async-hooks/test-timers.setTimeout.js new file mode 100644 index 00000000000000..8f1d3222ddf662 --- /dev/null +++ b/test/async-hooks/test-timers.setTimeout.js @@ -0,0 +1,61 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const tick = require('./tick'); +const initHooks = require('./init-hooks'); +const { checkInvocations } = require('./hook-checks'); +const TIMEOUT = common.platformTimeout(100); + +const hooks = initHooks(); +hooks.enable(); + +// install first timeout +setTimeout(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 first timer installed'); + +let timer; +let t2; +function ontimeout() { + checkInvocations(t1, { init: 1, before: 1 }, 't1: when first timer fired'); + + setTimeout(onSecondTimeout, TIMEOUT).unref(); + const as = hooks.activitiesOfTypes('Timeout'); + t2 = as[1]; + assert.strictEqual(as.length, 2); + checkInvocations(t1, { init: 1, before: 1 }, + 't1: when second timer installed'); + checkInvocations(t2, { init: 1 }, + 't2: when second timer installed'); + + timer = setTimeout(common.mustNotCall(), 2 ** 31 - 1); +} + +function onSecondTimeout() { + const as = hooks.activitiesOfTypes('Timeout'); + assert.strictEqual(as.length, 3); + checkInvocations(t1, { init: 1, before: 1, after: 1 }, + 't1: when second timer fired'); + checkInvocations(t2, { init: 1, before: 1 }, + 't2: when second timer fired'); + clearTimeout(timer); + tick(2); +} + +process.on('exit', onexit); + +function onexit() { + hooks.disable(); + hooks.sanityCheck('Timeout'); + + checkInvocations(t1, { init: 1, before: 1, after: 1, destroy: 1 }, + 't1: when process exits'); + checkInvocations(t2, { init: 1, before: 1, after: 1, destroy: 1 }, + 't2: when process exits'); +} From 05b7a97e44b0ccb5e7917b3a458a72e4c998e5fe Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 5 Feb 2018 13:09:08 -0500 Subject: [PATCH 4/4] timers: simplify clearTimeout & clearInterval Remove unnecessary condition from timeout & interval clearing. --- lib/timers.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/timers.js b/lib/timers.js index 1ea4c603719c6f..8e2ddd3a6f93ad 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -465,8 +465,8 @@ function rearm(timer, start = TimerWrap.now()) { const clearTimeout = exports.clearTimeout = function(timer) { - if (timer && (timer[kOnTimeout] || timer._onTimeout)) { - timer[kOnTimeout] = timer._onTimeout = null; + if (timer && timer._onTimeout) { + timer._onTimeout = null; if (timer instanceof Timeout) { timer.close(); // for after === 0 } else {