From bb5575aa75fd3071724d5eccde39a3041e1af57a Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Tue, 9 Jan 2018 13:12:06 -0500 Subject: [PATCH] timers: add internal [@@ refresh()] function Hidden via a symbol because I'm unsure exactly what the API should look like in the end. Removes the need to use _unrefActive for efficiently refreshing timeouts. It still uses it under the hood but that could be replaced with insert() directly if it were in the same file. PR-URL: https://github.com/nodejs/node/pull/18065 Reviewed-By: Anatoli Papirovski --- lib/internal/http2/core.js | 9 ++--- lib/internal/timers.js | 23 ++++++++++- lib/net.js | 6 +-- lib/timers.js | 6 +-- test/parallel/test-timers-refresh.js | 59 ++++++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 13 deletions(-) create mode 100644 test/parallel/test-timers-refresh.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 7c5792c6ce101d..595030a13a49f5 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -57,11 +57,10 @@ const { const { kTimeout, setUnrefTimeout, - validateTimerDuration + validateTimerDuration, + refreshFnSymbol } = require('internal/timers'); -const { _unrefActive } = require('timers'); - const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap'); const { constants } = binding; @@ -912,7 +911,7 @@ class Http2Session extends EventEmitter { [kUpdateTimer]() { if (this.destroyed) return; - if (this[kTimeout]) _unrefActive(this[kTimeout]); + if (this[kTimeout]) this[kTimeout][refreshFnSymbol](); } // Sets the id of the next stream to be created by this Http2Session. @@ -1478,7 +1477,7 @@ class Http2Stream extends Duplex { if (this.destroyed) return; if (this[kTimeout]) - _unrefActive(this[kTimeout]); + this[kTimeout][refreshFnSymbol](); if (this[kSession]) this[kSession][kUpdateTimer](); } diff --git a/lib/internal/timers.js b/lib/internal/timers.js index cd5efb6dbb9b92..aa061be3dbf845 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -19,12 +19,16 @@ const errors = require('internal/errors'); // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2 ** 31 - 1; +const refreshFnSymbol = Symbol('refresh()'); +const unrefedSymbol = Symbol('unrefed'); + module.exports = { TIMEOUT_MAX, kTimeout: Symbol('timeout'), // For hiding Timeouts on other internals. async_id_symbol, trigger_async_id_symbol, Timeout, + refreshFnSymbol, setUnrefTimeout, validateTimerDuration }; @@ -39,7 +43,7 @@ function getTimers() { // Timer constructor function. // The entire prototype is defined in lib/timers.js -function Timeout(callback, after, args, isRepeat) { +function Timeout(callback, after, args, isRepeat, isUnrefed) { after *= 1; // coalesce to number or NaN if (!(after >= 1 && after <= TIMEOUT_MAX)) { if (after > TIMEOUT_MAX) { @@ -64,6 +68,8 @@ function Timeout(callback, after, args, isRepeat) { this._repeat = isRepeat ? after : null; this._destroyed = false; + this[unrefedSymbol] = isUnrefed; + this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter]; this[trigger_async_id_symbol] = getDefaultTriggerAsyncId(); if (async_hook_fields[kInit] > 0) { @@ -74,6 +80,19 @@ function Timeout(callback, after, args, isRepeat) { } } +Timeout.prototype[refreshFnSymbol] = function refresh() { + if (this._handle) { + // Would be more ideal with uv_timer_again(), however that API does not + // cause libuv's sorted timers data structure (a binary heap at the time + // of writing) to re-sort itself. This causes ordering inconsistencies. + this._handle.stop(); + this._handle.start(this._idleTimeout); + } else if (this[unrefedSymbol]) { + getTimers()._unrefActive(this); + } else { + getTimers().active(this); + } +}; function setUnrefTimeout(callback, after, arg1, arg2, arg3) { // Type checking identical to setTimeout() @@ -102,7 +121,7 @@ function setUnrefTimeout(callback, after, arg1, arg2, arg3) { break; } - const timer = new Timeout(callback, after, args, false); + const timer = new Timeout(callback, after, args, false, true); getTimers()._unrefActive(timer); return timer; diff --git a/lib/net.js b/lib/net.js index 16d0b9605c594a..41c1cacac5e6fd 100644 --- a/lib/net.js +++ b/lib/net.js @@ -23,7 +23,6 @@ const EventEmitter = require('events'); const stream = require('stream'); -const timers = require('timers'); const util = require('util'); const internalUtil = require('internal/util'); const { @@ -64,7 +63,8 @@ const exceptionWithHostPort = util._exceptionWithHostPort; const { kTimeout, setUnrefTimeout, - validateTimerDuration + validateTimerDuration, + refreshFnSymbol } = require('internal/timers'); function noop() {} @@ -291,7 +291,7 @@ util.inherits(Socket, stream.Duplex); Socket.prototype._unrefTimer = function _unrefTimer() { for (var s = this; s !== null; s = s._parent) { if (s[kTimeout]) - timers._unrefActive(s[kTimeout]); + s[kTimeout][refreshFnSymbol](); } }; diff --git a/lib/timers.js b/lib/timers.js index 6fc552fac7d444..a38829d2f02bc7 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -432,7 +432,7 @@ function setTimeout(callback, after, arg1, arg2, arg3) { break; } - const timeout = new Timeout(callback, after, args, false); + const timeout = new Timeout(callback, after, args, false, false); active(timeout); return timeout; @@ -440,7 +440,7 @@ function setTimeout(callback, after, arg1, arg2, arg3) { setTimeout[internalUtil.promisify.custom] = function(after, value) { const promise = createPromise(); - const timeout = new Timeout(promise, after, [value], false); + const timeout = new Timeout(promise, after, [value], false, false); active(timeout); return promise; @@ -523,7 +523,7 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) { break; } - const timeout = new Timeout(callback, repeat, args, true); + const timeout = new Timeout(callback, repeat, args, true, false); active(timeout); return timeout; diff --git a/test/parallel/test-timers-refresh.js b/test/parallel/test-timers-refresh.js new file mode 100644 index 00000000000000..25a22329c01ee9 --- /dev/null +++ b/test/parallel/test-timers-refresh.js @@ -0,0 +1,59 @@ +// Flags: --expose-internals + +'use strict'; + +const common = require('../common'); + +const { strictEqual } = require('assert'); +const { setUnrefTimeout, refreshFnSymbol } = require('internal/timers'); + +// Schedule the unrefed cases first so that the later case keeps the event loop +// active. + +// Every case in this test relies on implicit sorting within either Node's or +// libuv's timers storage data structures. + +// unref()'d timer +{ + let called = false; + const timer = setTimeout(common.mustCall(() => { + called = true; + }), 1); + timer.unref(); + + // This relies on implicit timers handle sorting withing libuv. + + setTimeout(common.mustCall(() => { + strictEqual(called, false, 'unref()\'d timer returned before check'); + }), 1); + + timer[refreshFnSymbol](); +} + +// unref pooled timer +{ + let called = false; + const timer = setUnrefTimeout(common.mustCall(() => { + called = true; + }), 1); + + setUnrefTimeout(common.mustCall(() => { + strictEqual(called, false, 'unref pooled timer returned before check'); + }), 1); + + timer[refreshFnSymbol](); +} + +// regular timer +{ + let called = false; + const timer = setTimeout(common.mustCall(() => { + called = true; + }), 1); + + setTimeout(common.mustCall(() => { + strictEqual(called, false, 'pooled timer returned before check'); + }), 1); + + timer[refreshFnSymbol](); +}