From 24dd92e77f9aad95ce3cae0baba75d0a5e5439cf Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 3 Feb 2017 16:05:59 -0500 Subject: [PATCH] net: use actual Timeout instance on Sockets This makes `net.Sockets` use actual Timeout objects in a `[kTimeout]` symbol property, rather than making the socket itself a timer and appending properties to it directly. This should make the code generally easier to understand, and might also prevent some deopts from properties being changes on the socket itself. Also moves the Timeout constructor into an internal module. PR-URL: https://github.com/nodejs/node/pull/17704 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Anatoli Papirovski --- lib/internal/timers.js | 107 ++++++++++++++++++ lib/net.js | 43 +++++-- lib/timers.js | 62 +++------- node.gyp | 1 + .../test-http-client-timeout-on-connect.js | 8 +- test/parallel/test-net-socket-timeout.js | 4 +- test/parallel/test-tls-wrap-timeout.js | 13 ++- 7 files changed, 178 insertions(+), 60 deletions(-) create mode 100644 lib/internal/timers.js diff --git a/lib/internal/timers.js b/lib/internal/timers.js new file mode 100644 index 00000000000000..677da4e8ea8543 --- /dev/null +++ b/lib/internal/timers.js @@ -0,0 +1,107 @@ +'use strict'; + +const async_wrap = process.binding('async_wrap'); +// Two arrays that share state between C++ and JS. +const { async_hook_fields, async_id_fields } = async_wrap; +const { + getDefaultTriggerAsyncId, + // The needed emit*() functions. + emitInit +} = require('internal/async_hooks'); +// Grab the constants necessary for working with internal arrays. +const { kInit, kAsyncIdCounter } = async_wrap.constants; +// Symbols for storing async id state. +const async_id_symbol = Symbol('asyncId'); +const trigger_async_id_symbol = Symbol('triggerId'); + +const errors = require('internal/errors'); + +// Timeout values > TIMEOUT_MAX are set to 1. +const TIMEOUT_MAX = 2 ** 31 - 1; + +module.exports = { + TIMEOUT_MAX, + kTimeout: Symbol('timeout'), // For hiding Timeouts on other internals. + async_id_symbol, + trigger_async_id_symbol, + Timeout, + setUnrefTimeout, +}; + +// Timer constructor function. +// The entire prototype is defined in lib/timers.js +function Timeout(callback, after, args, isRepeat) { + after *= 1; // coalesce to number or NaN + if (!(after >= 1 && after <= TIMEOUT_MAX)) { + if (after > TIMEOUT_MAX) { + process.emitWarning(`${after} does not fit into` + + ' a 32-bit signed integer.' + + '\nTimeout duration was set to 1.', + 'TimeoutOverflowWarning'); + } + after = 1; // schedule on next tick, follows browser behavior + } + + this._called = false; + this._idleTimeout = after; + this._idlePrev = this; + this._idleNext = this; + this._idleStart = null; + // this must be set to null first to avoid function tracking + // on the hidden class, revisit in V8 versions after 6.2 + this._onTimeout = null; + this._onTimeout = callback; + this._timerArgs = args; + this._repeat = isRepeat ? after : null; + this._destroyed = false; + + this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter]; + this[trigger_async_id_symbol] = getDefaultTriggerAsyncId(); + if (async_hook_fields[kInit] > 0) { + emitInit(this[async_id_symbol], + 'Timeout', + this[trigger_async_id_symbol], + this); + } +} + +var timers; +function getTimers() { + if (timers === undefined) { + timers = require('timers'); + } + return timers; +} + +function setUnrefTimeout(callback, after, arg1, arg2, arg3) { + // Type checking identical to setTimeout() + if (typeof callback !== 'function') { + throw new errors.TypeError('ERR_INVALID_CALLBACK'); + } + + let i, args; + switch (arguments.length) { + // fast cases + case 1: + case 2: + break; + case 3: + args = [arg1]; + break; + case 4: + args = [arg1, arg2]; + break; + default: + args = [arg1, arg2, arg3]; + for (i = 5; i < arguments.length; i++) { + // extend array dynamically, makes .apply run much faster in v6.0.0 + args[i - 2] = arguments[i]; + } + break; + } + + const timer = new Timeout(callback, after, args, false); + getTimers()._unrefActive(timer); + + return timer; +} diff --git a/lib/net.js b/lib/net.js index ce87a60ce0e78b..b6cfa30c69b554 100644 --- a/lib/net.js +++ b/lib/net.js @@ -57,6 +57,8 @@ var cluster = null; const errnoException = util._errnoException; const exceptionWithHostPort = util._exceptionWithHostPort; +const { kTimeout, TIMEOUT_MAX, setUnrefTimeout } = require('internal/timers'); + function noop() {} function createHandle(fd, is_server) { @@ -201,6 +203,7 @@ function Socket(options) { this._parent = null; this._host = null; this[kLastWriteQueueSize] = 0; + this[kTimeout] = null; if (typeof options === 'number') options = { fd: options }; // Legacy interface. @@ -272,9 +275,12 @@ function Socket(options) { } util.inherits(Socket, stream.Duplex); +// Refresh existing timeouts. Socket.prototype._unrefTimer = function _unrefTimer() { - for (var s = this; s !== null; s = s._parent) - timers._unrefActive(s); + for (var s = this; s !== null; s = s._parent) { + if (s[kTimeout]) + timers._unrefActive(s[kTimeout]); + } }; @@ -387,14 +393,36 @@ Socket.prototype.read = function(n) { }; Socket.prototype.setTimeout = function(msecs, callback) { + // Type checking identical to timers.enroll() + if (typeof msecs !== 'number') { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'msecs', + 'number', msecs); + } + + if (msecs < 0 || !isFinite(msecs)) { + throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE', 'msecs', + 'a non-negative finite number', msecs); + } + + // Ensure that msecs fits into signed int32 + if (msecs > TIMEOUT_MAX) { + process.emitWarning(`${msecs} does not fit into a 32-bit signed integer.` + + `\nTimer duration was truncated to ${TIMEOUT_MAX}.`, + 'TimeoutOverflowWarning'); + msecs = TIMEOUT_MAX; + } + + // Attempt to clear an existing timer lear in both cases - + // even if it will be rescheduled we don't want to leak an existing timer. + clearTimeout(this[kTimeout]); + if (msecs === 0) { - timers.unenroll(this); if (callback) { this.removeListener('timeout', callback); } } else { - timers.enroll(this, msecs); - timers._unrefActive(this); + this[kTimeout] = setUnrefTimeout(this._onTimeout.bind(this), msecs); + if (callback) { this.once('timeout', callback); } @@ -551,8 +579,9 @@ Socket.prototype._destroy = function(exception, cb) { this.readable = this.writable = false; - for (var s = this; s !== null; s = s._parent) - timers.unenroll(s); + for (var s = this; s !== null; s = s._parent) { + clearTimeout(s[kTimeout]); + } debug('close'); if (this._handle) { diff --git a/lib/timers.js b/lib/timers.js index 479463d2d0ad4c..e9a39ee281b0aa 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -24,6 +24,7 @@ const async_wrap = process.binding('async_wrap'); const TimerWrap = process.binding('timer_wrap').Timer; const L = require('internal/linkedlist'); +const timerInternals = require('internal/timers'); const internalUtil = require('internal/util'); const { createPromise, promiseResolve } = process.binding('util'); const assert = require('assert'); @@ -44,8 +45,8 @@ const { // Grab the constants necessary for working with internal arrays. const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants; // Symbols for storing async id state. -const async_id_symbol = Symbol('asyncId'); -const trigger_async_id_symbol = Symbol('triggerAsyncId'); +const async_id_symbol = timerInternals.async_id_symbol; +const trigger_async_id_symbol = timerInternals.trigger_async_id_symbol; /* This is an Uint32Array for easier sharing with C++ land. */ const scheduledImmediateCount = process._scheduledImmediateCount; @@ -55,7 +56,10 @@ const activateImmediateCheck = process._activateImmediateCheck; delete process._activateImmediateCheck; // Timeout values > TIMEOUT_MAX are set to 1. -const TIMEOUT_MAX = 2 ** 31 - 1; +const TIMEOUT_MAX = timerInternals.TIMEOUT_MAX; + +// The Timeout class +const Timeout = timerInternals.Timeout; // HOW and WHY the timers implementation works the way it does. @@ -446,12 +450,17 @@ function setTimeout(callback, after, arg1, arg2, arg3) { break; } - return new Timeout(callback, after, args, false); + const timeout = new Timeout(callback, after, args, false); + active(timeout); + + return timeout; } setTimeout[internalUtil.promisify.custom] = function(after, value) { const promise = createPromise(); - new Timeout(promise, after, [value], false); + const timeout = new Timeout(promise, after, [value], false); + active(timeout); + return promise; }; @@ -523,7 +532,10 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) { break; } - return new Timeout(callback, repeat, args, true); + const timeout = new Timeout(callback, repeat, args, true); + active(timeout); + + return timeout; }; exports.clearInterval = function(timer) { @@ -534,44 +546,6 @@ exports.clearInterval = function(timer) { }; -function Timeout(callback, after, args, isRepeat) { - after *= 1; // coalesce to number or NaN - if (!(after >= 1 && after <= TIMEOUT_MAX)) { - if (after > TIMEOUT_MAX) { - process.emitWarning(`${after} does not fit into` + - ' a 32-bit signed integer.' + - '\nTimeout duration was set to 1.', - 'TimeoutOverflowWarning'); - } - after = 1; // schedule on next tick, follows browser behavior - } - - this._called = false; - this._idleTimeout = after; - this._idlePrev = this; - this._idleNext = this; - this._idleStart = null; - // this must be set to null first to avoid function tracking - // on the hidden class, revisit in V8 versions after 6.2 - this._onTimeout = null; - this._onTimeout = callback; - this._timerArgs = args; - this._repeat = isRepeat ? after : null; - this._destroyed = false; - - this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter]; - this[trigger_async_id_symbol] = getDefaultTriggerAsyncId(); - if (async_hook_fields[kInit] > 0) { - emitInit(this[async_id_symbol], - 'Timeout', - this[trigger_async_id_symbol], - this); - } - - active(this); -} - - function unrefdHandle() { // Don't attempt to call the callback if it is not a function. if (typeof this.owner._onTimeout === 'function') { diff --git a/node.gyp b/node.gyp index 5d4650e560ccfc..c1c83f6213f262 100644 --- a/node.gyp +++ b/node.gyp @@ -123,6 +123,7 @@ 'lib/internal/repl/await.js', 'lib/internal/socket_list.js', 'lib/internal/test/unicode.js', + 'lib/internal/timers.js', 'lib/internal/tls.js', 'lib/internal/trace_events_async_hooks.js', 'lib/internal/url.js', diff --git a/test/parallel/test-http-client-timeout-on-connect.js b/test/parallel/test-http-client-timeout-on-connect.js index af3b3ef53debc2..928f781e261758 100644 --- a/test/parallel/test-http-client-timeout-on-connect.js +++ b/test/parallel/test-http-client-timeout-on-connect.js @@ -1,7 +1,11 @@ +// Flags: --expose-internals + 'use strict'; + const common = require('../common'); const assert = require('assert'); const http = require('http'); +const { kTimeout } = require('internal/timers'); const server = http.createServer((req, res) => { // This space is intentionally left blank. @@ -13,9 +17,9 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => { req.setTimeout(1); req.on('socket', common.mustCall((socket) => { - assert.strictEqual(socket._idleTimeout, undefined); + assert.strictEqual(socket[kTimeout], null); socket.on('connect', common.mustCall(() => { - assert.strictEqual(socket._idleTimeout, 1); + assert.strictEqual(socket[kTimeout]._idleTimeout, 1); })); })); req.on('timeout', common.mustCall(() => req.abort())); diff --git a/test/parallel/test-net-socket-timeout.js b/test/parallel/test-net-socket-timeout.js index de4a7ed37ccf20..178e2d994daab0 100644 --- a/test/parallel/test-net-socket-timeout.js +++ b/test/parallel/test-net-socket-timeout.js @@ -36,13 +36,13 @@ const validDelays = [0, 0.001, 1, 1e6]; for (let i = 0; i < nonNumericDelays.length; i++) { assert.throws(function() { s.setTimeout(nonNumericDelays[i], () => {}); - }, TypeError); + }, TypeError, nonNumericDelays[i]); } for (let i = 0; i < badRangeDelays.length; i++) { assert.throws(function() { s.setTimeout(badRangeDelays[i], () => {}); - }, RangeError); + }, RangeError, badRangeDelays[i]); } for (let i = 0; i < validDelays.length; i++) { diff --git a/test/parallel/test-tls-wrap-timeout.js b/test/parallel/test-tls-wrap-timeout.js index d1598ab737f129..6ae2c39c59b8d9 100644 --- a/test/parallel/test-tls-wrap-timeout.js +++ b/test/parallel/test-tls-wrap-timeout.js @@ -1,5 +1,8 @@ +// Flags: --expose_internals + 'use strict'; const common = require('../common'); +const { kTimeout, TIMEOUT_MAX } = require('internal/timers'); if (!common.hasCrypto) common.skip('missing crypto'); @@ -30,13 +33,13 @@ let lastIdleStart; server.listen(0, () => { socket = net.connect(server.address().port, function() { - const s = socket.setTimeout(Number.MAX_VALUE, function() { + const s = socket.setTimeout(TIMEOUT_MAX, function() { throw new Error('timeout'); }); assert.ok(s instanceof net.Socket); - assert.notStrictEqual(socket._idleTimeout, -1); - lastIdleStart = socket._idleStart; + assert.notStrictEqual(socket[kTimeout]._idleTimeout, -1); + lastIdleStart = socket[kTimeout]._idleStart; const tsocket = tls.connect({ socket: socket, @@ -47,6 +50,6 @@ server.listen(0, () => { }); process.on('exit', () => { - assert.strictEqual(socket._idleTimeout, -1); - assert(lastIdleStart < socket._idleStart); + assert.strictEqual(socket[kTimeout]._idleTimeout, -1); + assert(lastIdleStart < socket[kTimeout]._idleStart); });