http2: use actual Timeout instances

This makes `Http2Stream`s and `Http2Session`s use actual Timeout
objects in a [kTimeout] symbol property, rather than making the
stream/session itself a timer and appending properties to it directly.

PR-URL: https://github.com/nodejs/node/pull/17704
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
This commit is contained in:
Jeremiah Senkpiel 2017-12-17 00:21:37 -05:00
parent 593941ac0b
commit 93eb68e6d2
4 changed files with 46 additions and 29 deletions

View File

@ -52,10 +52,12 @@ const {
} = require('internal/http2/util'); } = require('internal/http2/util');
const { const {
_unrefActive, kTimeout,
enroll, setUnrefTimeout,
unenroll validateTimerDuration
} = require('timers'); } = require('internal/timers');
const { _unrefActive } = require('timers');
const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap'); const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap');
const { constants } = binding; const { constants } = binding;
@ -280,8 +282,8 @@ function onStreamClose(code, hasData) {
` [has data? ${hasData}]`); ` [has data? ${hasData}]`);
if (!stream.closed) { if (!stream.closed) {
// Unenroll from timeouts // Clear timeout and remove timeout listeners
unenroll(stream); stream.setTimeout(0);
stream.removeAllListeners('timeout'); stream.removeAllListeners('timeout');
// Set the state flags // Set the state flags
@ -788,6 +790,7 @@ class Http2Session extends EventEmitter {
this[kType] = type; this[kType] = type;
this[kProxySocket] = null; this[kProxySocket] = null;
this[kSocket] = socket; this[kSocket] = socket;
this[kTimeout] = null;
// Do not use nagle's algorithm // Do not use nagle's algorithm
if (typeof socket.setNoDelay === 'function') if (typeof socket.setNoDelay === 'function')
@ -828,7 +831,7 @@ class Http2Session extends EventEmitter {
[kUpdateTimer]() { [kUpdateTimer]() {
if (this.destroyed) if (this.destroyed)
return; return;
_unrefActive(this); if (this[kTimeout]) _unrefActive(this[kTimeout]);
} }
// Sets the id of the next stream to be created by this Http2Session. // Sets the id of the next stream to be created by this Http2Session.
@ -1019,7 +1022,7 @@ class Http2Session extends EventEmitter {
state.flags |= SESSION_FLAGS_DESTROYED; state.flags |= SESSION_FLAGS_DESTROYED;
// Clear timeout and remove timeout listeners // Clear timeout and remove timeout listeners
unenroll(this); this.setTimeout(0);
this.removeAllListeners('timeout'); this.removeAllListeners('timeout');
// Destroy any pending and open streams // Destroy any pending and open streams
@ -1322,6 +1325,8 @@ class Http2Stream extends Duplex {
this[kSession] = session; this[kSession] = session;
session[kState].pendingStreams.add(this); session[kState].pendingStreams.add(this);
this[kTimeout] = null;
this[kState] = { this[kState] = {
flags: STREAM_FLAGS_PENDING, flags: STREAM_FLAGS_PENDING,
rstCode: NGHTTP2_NO_ERROR, rstCode: NGHTTP2_NO_ERROR,
@ -1336,9 +1341,10 @@ class Http2Stream extends Duplex {
[kUpdateTimer]() { [kUpdateTimer]() {
if (this.destroyed) if (this.destroyed)
return; return;
_unrefActive(this); if (this[kTimeout])
if (this[kSession]) _unrefActive([kTimeout]);
_unrefActive(this[kSession]); if (this[kSession] && this[kSession][kTimeout])
_unrefActive(this[kSession][kTimeout]);
} }
[kInit](id, handle) { [kInit](id, handle) {
@ -1560,7 +1566,7 @@ class Http2Stream extends Duplex {
// Close initiates closing the Http2Stream instance by sending an RST_STREAM // Close initiates closing the Http2Stream instance by sending an RST_STREAM
// frame to the connected peer. The readable and writable sides of the // frame to the connected peer. The readable and writable sides of the
// Http2Stream duplex are closed and the timeout timer is unenrolled. If // Http2Stream duplex are closed and the timeout timer is cleared. If
// a callback is passed, it is registered to listen for the 'close' event. // a callback is passed, it is registered to listen for the 'close' event.
// //
// If the handle and stream ID have not been assigned yet, the close // If the handle and stream ID have not been assigned yet, the close
@ -1577,8 +1583,8 @@ class Http2Stream extends Duplex {
if (code < 0 || code > kMaxInt) if (code < 0 || code > kMaxInt)
throw new errors.RangeError('ERR_OUT_OF_RANGE', 'code'); throw new errors.RangeError('ERR_OUT_OF_RANGE', 'code');
// Unenroll the timeout. // Clear timeout and remove timeout listeners
unenroll(this); this.setTimeout(0);
this.removeAllListeners('timeout'); this.removeAllListeners('timeout');
// Close the writable // Close the writable
@ -1637,8 +1643,10 @@ class Http2Stream extends Duplex {
handle.destroy(); handle.destroy();
session[kState].streams.delete(id); session[kState].streams.delete(id);
} else { } else {
unenroll(this); // Clear timeout and remove timeout listeners
this.setTimeout(0);
this.removeAllListeners('timeout'); this.removeAllListeners('timeout');
state.flags |= STREAM_FLAGS_CLOSED; state.flags |= STREAM_FLAGS_CLOSED;
abort(this); abort(this);
this.end(); this.end();
@ -2216,21 +2224,24 @@ const setTimeout = {
value: function(msecs, callback) { value: function(msecs, callback) {
if (this.destroyed) if (this.destroyed)
return; return;
if (typeof msecs !== 'number') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', // Type checking identical to timers.enroll()
'msecs', msecs = validateTimerDuration(msecs);
'number');
} // 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) { if (msecs === 0) {
unenroll(this);
if (callback !== undefined) { if (callback !== undefined) {
if (typeof callback !== 'function') if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK'); throw new errors.TypeError('ERR_INVALID_CALLBACK');
this.removeListener('timeout', callback); this.removeListener('timeout', callback);
} }
} else { } else {
enroll(this, msecs); this[kTimeout] = setUnrefTimeout(this._onTimeout.bind(this), msecs);
this[kUpdateTimer](); if (this[kSession]) this[kSession][kUpdateTimer]();
if (callback !== undefined) { if (callback !== undefined) {
if (typeof callback !== 'function') if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK'); throw new errors.TypeError('ERR_INVALID_CALLBACK');

View File

@ -1,3 +1,5 @@
// Flags: --expose_internals
'use strict'; 'use strict';
const common = require('../common'); const common = require('../common');
@ -7,6 +9,8 @@ const assert = require('assert');
const h2 = require('http2'); const h2 = require('http2');
const net = require('net'); const net = require('net');
const { kTimeout } = require('internal/timers');
// Tests behavior of the proxied socket in Http2ServerRequest // Tests behavior of the proxied socket in Http2ServerRequest
// & Http2ServerResponse - this proxy socket should mimic the // & Http2ServerResponse - this proxy socket should mimic the
// behavior of http1 but against the http2 api & model // behavior of http1 but against the http2 api & model
@ -31,7 +35,7 @@ server.on('request', common.mustCall(function(request, response) {
assert.strictEqual(request.socket.destroyed, false); assert.strictEqual(request.socket.destroyed, false);
request.socket.setTimeout(987); request.socket.setTimeout(987);
assert.strictEqual(request.stream.session._idleTimeout, 987); assert.strictEqual(request.stream.session[kTimeout]._idleTimeout, 987);
request.socket.setTimeout(0); request.socket.setTimeout(0);
common.expectsError(() => request.socket.read(), errMsg); common.expectsError(() => request.socket.read(), errMsg);

View File

@ -1,3 +1,5 @@
// Flags: --expose_internals
'use strict'; 'use strict';
const common = require('../common'); const common = require('../common');
@ -7,6 +9,8 @@ const assert = require('assert');
const h2 = require('http2'); const h2 = require('http2');
const net = require('net'); const net = require('net');
const { kTimeout } = require('internal/timers');
// Tests behavior of the proxied socket on Http2Session // Tests behavior of the proxied socket on Http2Session
const errMsg = { const errMsg = {
@ -29,7 +33,7 @@ server.on('stream', common.mustCall(function(stream, headers) {
assert.strictEqual(typeof socket.address(), 'object'); assert.strictEqual(typeof socket.address(), 'object');
socket.setTimeout(987); socket.setTimeout(987);
assert.strictEqual(session._idleTimeout, 987); assert.strictEqual(session[kTimeout]._idleTimeout, 987);
common.expectsError(() => socket.destroy, errMsg); common.expectsError(() => socket.destroy, errMsg);
common.expectsError(() => socket.emit, errMsg); common.expectsError(() => socket.emit, errMsg);
@ -59,9 +63,6 @@ server.on('stream', common.mustCall(function(stream, headers) {
stream.end(); stream.end();
socket.setTimeout = undefined;
assert.strictEqual(session.setTimeout, undefined);
stream.session.on('close', common.mustCall(() => { stream.session.on('close', common.mustCall(() => {
assert.strictEqual(session.socket, undefined); assert.strictEqual(session.socket, undefined);
})); }));

View File

@ -20,7 +20,8 @@ server.on('stream', common.mustCall((stream) => {
{ {
code: 'ERR_INVALID_ARG_TYPE', code: 'ERR_INVALID_ARG_TYPE',
type: TypeError, type: TypeError,
message: 'The "msecs" argument must be of type number' message:
'The "msecs" argument must be of type number. Received type string'
} }
); );
common.expectsError( common.expectsError(