From 21190b5ec57555a8ce6442d553361f4460e0ae40 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 22 Apr 2019 10:30:39 -0700 Subject: [PATCH 1/3] timers: do less work in insert Most of the code in insert is only applicable to scheduling non-timers or re-scheduling timers. We can skip most of it in the case of setTimeout, setInterval & setUnrefTimeout. --- lib/internal/timers.js | 58 ++++++++++++++++++++++-------------------- lib/timers.js | 14 +++++----- 2 files changed, 39 insertions(+), 33 deletions(-) diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 1b6422f41ea7af..7bd3f809b9bda3 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -155,7 +155,7 @@ function initAsyncResource(resource, type) { // Timer constructor function. // The entire prototype is defined in lib/timers.js -function Timeout(callback, after, args, isRepeat) { +function Timeout(callback, after, args, isRepeat, isRefed) { after *= 1; // Coalesce to number or NaN if (!(after >= 1 && after <= TIMEOUT_MAX)) { if (after > TIMEOUT_MAX) { @@ -179,7 +179,9 @@ function Timeout(callback, after, args, isRepeat) { this._repeat = isRepeat ? after : null; this._destroyed = false; - this[kRefed] = null; + if (isRefed) + incRefCount(); + this[kRefed] = isRefed; initAsyncResource(this, 'Timeout'); } @@ -295,27 +297,43 @@ function decRefCount() { // Schedule or re-schedule a timer. // The item must have been enroll()'d first. function active(item) { - insert(item, true, getLibuvNow()); + insertGuarded(item, true); } // Internal APIs that need timeouts should use `unrefActive()` instead of // `active()` so that they do not unnecessarily keep the process open. function unrefActive(item) { - insert(item, false, getLibuvNow()); + insertGuarded(item, false); } // The underlying logic for scheduling or re-scheduling a timer. // // Appends a timer onto the end of an existing timers list, or creates a new // list if one does not already exist for the specified timeout duration. -function insert(item, refed, start) { - let msecs = item._idleTimeout; +function insertGuarded(item, refed, start) { + const msecs = item._idleTimeout; if (msecs < 0 || msecs === undefined) return; - // Truncate so that accuracy of sub-millisecond timers is not assumed. - msecs = MathTrunc(msecs); + insert(item, msecs, start); + + if (!item[async_id_symbol] || item._destroyed) { + item._destroyed = false; + initAsyncResource(item, 'Timeout'); + } + + if (refed === !item[kRefed]) { + if (refed) + incRefCount(); + else + decRefCount(); + } + item[kRefed] = refed; +} +function insert(item, msecs, start = getLibuvNow()) { + // Truncate so that accuracy of sub-milisecond timers is not assumed. + msecs = MathTrunc(msecs); item._idleStart = start; // Use an existing list if there is one, otherwise we need to make a new one. @@ -332,19 +350,6 @@ function insert(item, refed, start) { } } - if (!item[async_id_symbol] || item._destroyed) { - item._destroyed = false; - initAsyncResource(item, 'Timeout'); - } - - if (refed === !item[kRefed]) { - if (refed) - incRefCount(); - else - decRefCount(); - } - item[kRefed] = refed; - L.append(list, item); } @@ -354,8 +359,8 @@ function setUnrefTimeout(callback, after) { throw new ERR_INVALID_CALLBACK(callback); } - const timer = new Timeout(callback, after, undefined, false); - unrefActive(timer); + const timer = new Timeout(callback, after, undefined, false, false); + insert(timer, timer._idleTimeout); return timer; } @@ -540,16 +545,14 @@ function getTimerCallbacks(runNextTicks) { } finally { if (timer._repeat && timer._idleTimeout !== -1) { timer._idleTimeout = timer._repeat; - if (start === undefined) - start = getLibuvNow(); - insert(timer, timer[kRefed], start); + insert(timer, timer._idleTimeout, start); } else if (!timer._idleNext && !timer._idlePrev) { if (timer[kRefed]) refCount--; timer[kRefed] = null; if (destroyHooksExist() && !timer._destroyed) { - emitDestroy(timer[async_id_symbol]); + emitDestroy(asyncId); } timer._destroyed = true; } @@ -598,6 +601,7 @@ module.exports = { }, active, unrefActive, + insert, timerListMap, timerListQueue, decRefCount, diff --git a/lib/timers.js b/lib/timers.js index 68327e2c3e5947..b732ea15fc59af 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -45,7 +45,8 @@ const { timerListQueue, immediateQueue, active, - unrefActive + unrefActive, + insert } = require('internal/timers'); const { promisify: { custom: customPromisify }, @@ -141,8 +142,8 @@ function setTimeout(callback, after, arg1, arg2, arg3) { break; } - const timeout = new Timeout(callback, after, args, false); - active(timeout); + const timeout = new Timeout(callback, after, args, false, true); + insert(timeout, timeout._idleTimeout); return timeout; } @@ -150,7 +151,8 @@ function setTimeout(callback, after, arg1, arg2, arg3) { setTimeout[customPromisify] = function(after, value) { const args = value !== undefined ? [value] : value; return new Promise((resolve) => { - active(new Timeout(resolve, after, args, false)); + const timeout = new Timeout(resolve, after, args, false, true); + insert(timeout, timeout._idleTimeout); }); }; @@ -187,8 +189,8 @@ function setInterval(callback, repeat, arg1, arg2, arg3) { break; } - const timeout = new Timeout(callback, repeat, args, true); - active(timeout); + const timeout = new Timeout(callback, repeat, args, true, true); + insert(timeout, timeout._idleTimeout); return timeout; } From 33b3eb443faeb09ada5248a36eef057737b93c54 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 22 Apr 2019 13:52:17 -0700 Subject: [PATCH 2/3] timers: fix refresh for expired timers Expired timers were not being refresh correctly and would always act as unrefed if refresh was called. --- lib/internal/timers.js | 36 +++++++++++++++------------- lib/timers.js | 12 +++++----- test/parallel/test-timers-refresh.js | 14 +++++++++++ 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 7bd3f809b9bda3..578691c6852605 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -207,7 +207,7 @@ Timeout.prototype.refresh = function() { }; Timeout.prototype.unref = function() { - if (this[kRefed]) { + if (this[kRefed] && !this._destroyed) { this[kRefed] = false; decRefCount(); } @@ -215,7 +215,7 @@ Timeout.prototype.unref = function() { }; Timeout.prototype.ref = function() { - if (this[kRefed] === false) { + if (!this[kRefed] && !this._destroyed) { this[kRefed] = true; incRefCount(); } @@ -223,7 +223,7 @@ Timeout.prototype.ref = function() { }; Timeout.prototype.hasRef = function() { - return !!this[kRefed]; + return !!(this[kRefed] && !this._destroyed); }; function TimersList(expiry, msecs) { @@ -317,12 +317,16 @@ function insertGuarded(item, refed, start) { insert(item, msecs, start); - if (!item[async_id_symbol] || item._destroyed) { + const isDestroyed = item._destroyed; + if (isDestroyed || !item[async_id_symbol]) { item._destroyed = false; initAsyncResource(item, 'Timeout'); } - if (refed === !item[kRefed]) { + if (isDestroyed) { + if (refed) + incRefCount(); + } else if (refed === !item[kRefed]) { if (refed) incRefCount(); else @@ -519,13 +523,14 @@ function getTimerCallbacks(runNextTicks) { const asyncId = timer[async_id_symbol]; if (!timer._onTimeout) { - if (timer[kRefed]) - refCount--; - timer[kRefed] = null; - - if (destroyHooksExist() && !timer._destroyed) { - emitDestroy(asyncId); + if (!timer._destroyed) { timer._destroyed = true; + + if (timer[kRefed]) + refCount--; + + if (destroyHooksExist()) + emitDestroy(asyncId); } continue; } @@ -546,15 +551,14 @@ function getTimerCallbacks(runNextTicks) { if (timer._repeat && timer._idleTimeout !== -1) { timer._idleTimeout = timer._repeat; insert(timer, timer._idleTimeout, start); - } else if (!timer._idleNext && !timer._idlePrev) { + } else if (!timer._idleNext && !timer._idlePrev && !timer._destroyed) { + timer._destroyed = true; + if (timer[kRefed]) refCount--; - timer[kRefed] = null; - if (destroyHooksExist() && !timer._destroyed) { + if (destroyHooksExist()) emitDestroy(asyncId); - } - timer._destroyed = true; } } diff --git a/lib/timers.js b/lib/timers.js index b732ea15fc59af..3377bb6c785da7 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -63,13 +63,14 @@ const { // Remove a timer. Cancels the timeout and resets the relevant timer properties. function unenroll(item) { + if (item._destroyed) + return; + + item._destroyed = true; + // Fewer checks may be possible, but these cover everything. - if (destroyHooksExist() && - item[async_id_symbol] !== undefined && - !item._destroyed) { + if (destroyHooksExist() && item[async_id_symbol] !== undefined) emitDestroy(item[async_id_symbol]); - } - item._destroyed = true; L.remove(item); @@ -90,7 +91,6 @@ function unenroll(item) { decRefCount(); } - item[kRefed] = null; // If active is called later, then we want to make sure not to insert again item._idleTimeout = -1; diff --git a/test/parallel/test-timers-refresh.js b/test/parallel/test-timers-refresh.js index e186c63a65ecf9..a96f5ad5368275 100644 --- a/test/parallel/test-timers-refresh.js +++ b/test/parallel/test-timers-refresh.js @@ -72,6 +72,20 @@ const { inspect } = require('util'); strictEqual(timer.refresh(), timer); } +// regular timer +{ + let called = false; + const timer = setTimeout(common.mustCall(() => { + if (!called) { + called = true; + process.nextTick(common.mustCall(() => { + timer.refresh(); + strictEqual(timer.hasRef(), true); + })); + } + }, 2), 1); +} + // interval { let called = 0; From d249fc238be34d039863d479cc7bf80269c7fc03 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 31 May 2019 13:09:11 +0200 Subject: [PATCH 3/3] fixup: ref/unref should still work when destroyed --- lib/internal/timers.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 578691c6852605..b62ab9499c8fff 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -207,23 +207,25 @@ Timeout.prototype.refresh = function() { }; Timeout.prototype.unref = function() { - if (this[kRefed] && !this._destroyed) { + if (this[kRefed]) { this[kRefed] = false; - decRefCount(); + if (!this._destroyed) + decRefCount(); } return this; }; Timeout.prototype.ref = function() { - if (!this[kRefed] && !this._destroyed) { + if (!this[kRefed]) { this[kRefed] = true; - incRefCount(); + if (!this._destroyed) + incRefCount(); } return this; }; Timeout.prototype.hasRef = function() { - return !!(this[kRefed] && !this._destroyed); + return this[kRefed]; }; function TimersList(expiry, msecs) {