FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: use actual Timeout instances · nodejs/node@93eb68e · GitHub

/ node Public

Commit 93eb68e

Browse files
Fishrock123
committed
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: #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>
1 parent 593941a commit 93eb68e

4 files changed

Lines changed: 46 additions & 29 deletions

File tree

‎lib/internal/http2/core.js‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,12 @@ const {
5252
} = require('internal/http2/util');
5353

5454
const {
55-
_unrefActive,
56-
enroll,
57-
unenroll
58-
} = require('timers');
55+
kTimeout,
56+
setUnrefTimeout,
57+
validateTimerDuration
58+
} = require('internal/timers');
59+
60+
const { _unrefActive } = require('timers');
5961

6062
const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap');
6163
const { constants } = binding;
@@ -280,8 +282,8 @@ function onStreamClose(code, hasData) {
280282
` [has data? ${hasData}]`);
281283

282284
if (!stream.closed) {
283-
// Unenroll from timeouts
284-
unenroll(stream);
285+
// Clear timeout and remove timeout listeners
286+
stream.setTimeout(0);
285287
stream.removeAllListeners('timeout');
286288

287289
// Set the state flags
@@ -788,6 +790,7 @@ class Http2Session extends EventEmitter {
788790
this[kType] = type;
789791
this[kProxySocket] = null;
790792
this[kSocket] = socket;
793+
this[kTimeout] = null;
791794

792795
// Do not use nagle's algorithm
793796
if (typeof socket.setNoDelay === 'function')
@@ -828,7 +831,7 @@ class Http2Session extends EventEmitter {
828831
[kUpdateTimer]() {
829832
if (this.destroyed)
830833
return;
831-
_unrefActive(this);
834+
if (this[kTimeout]) _unrefActive(this[kTimeout]);
832835
}
833836

834837
// Sets the id of the next stream to be created by this Http2Session.
@@ -1019,7 +1022,7 @@ class Http2Session extends EventEmitter {
10191022
state.flags |= SESSION_FLAGS_DESTROYED;
10201023

10211024
// Clear timeout and remove timeout listeners
1022-
unenroll(this);
1025+
this.setTimeout(0);
10231026
this.removeAllListeners('timeout');
10241027

10251028
// Destroy any pending and open streams
@@ -1322,6 +1325,8 @@ class Http2Stream extends Duplex {
13221325
this[kSession] = session;
13231326
session[kState].pendingStreams.add(this);
13241327

1328+
this[kTimeout] = null;
1329+
13251330
this[kState] = {
13261331
flags: STREAM_FLAGS_PENDING,
13271332
rstCode: NGHTTP2_NO_ERROR,
@@ -1336,9 +1341,10 @@ class Http2Stream extends Duplex {
13361341
[kUpdateTimer]() {
13371342
if (this.destroyed)
13381343
return;
1339-
_unrefActive(this);
1340-
if (this[kSession])
1341-
_unrefActive(this[kSession]);
1344+
if (this[kTimeout])
1345+
_unrefActive([kTimeout]);
1346+
if (this[kSession] && this[kSession][kTimeout])
1347+
_unrefActive(this[kSession][kTimeout]);
13421348
}
13431349

13441350
[kInit](id, handle) {
@@ -1560,7 +1566,7 @@ class Http2Stream extends Duplex {
15601566

15611567
// Close initiates closing the Http2Stream instance by sending an RST_STREAM
15621568
// frame to the connected peer. The readable and writable sides of the
1563-
// Http2Stream duplex are closed and the timeout timer is unenrolled. If
1569+
// Http2Stream duplex are closed and the timeout timer is cleared. If
15641570
// a callback is passed, it is registered to listen for the 'close' event.
15651571
//
15661572
// If the handle and stream ID have not been assigned yet, the close
@@ -1577,8 +1583,8 @@ class Http2Stream extends Duplex {
15771583
if (code < 0 || code > kMaxInt)
15781584
throw new errors.RangeError('ERR_OUT_OF_RANGE', 'code');
15791585

1580-
// Unenroll the timeout.
1581-
unenroll(this);
1586+
// Clear timeout and remove timeout listeners
1587+
this.setTimeout(0);
15821588
this.removeAllListeners('timeout');
15831589

15841590
// Close the writable
@@ -1637,8 +1643,10 @@ class Http2Stream extends Duplex {
16371643
handle.destroy();
16381644
session[kState].streams.delete(id);
16391645
} else {
1640-
unenroll(this);
1646+
// Clear timeout and remove timeout listeners
1647+
this.setTimeout(0);
16411648
this.removeAllListeners('timeout');
1649+
16421650
state.flags |= STREAM_FLAGS_CLOSED;
16431651
abort(this);
16441652
this.end();
@@ -2216,21 +2224,24 @@ const setTimeout = {
22162224
value: function(msecs, callback) {
22172225
if (this.destroyed)
22182226
return;
2219-
if (typeof msecs !== 'number') {
2220-
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
2221-
'msecs',
2222-
'number');
2223-
}
2227+
2228+
// Type checking identical to timers.enroll()
2229+
msecs = validateTimerDuration(msecs);
2230+
2231+
// Attempt to clear an existing timer lear in both cases -
2232+
// even if it will be rescheduled we don't want to leak an existing timer.
2233+
clearTimeout(this[kTimeout]);
2234+
22242235
if (msecs === 0) {
2225-
unenroll(this);
22262236
if (callback !== undefined) {
22272237
if (typeof callback !== 'function')
22282238
throw new errors.TypeError('ERR_INVALID_CALLBACK');
22292239
this.removeListener('timeout', callback);
22302240
}
22312241
} else {
2232-
enroll(this, msecs);
2233-
this[kUpdateTimer]();
2242+
this[kTimeout] = setUnrefTimeout(this._onTimeout.bind(this), msecs);
2243+
if (this[kSession]) this[kSession][kUpdateTimer]();
2244+
22342245
if (callback !== undefined) {
22352246
if (typeof callback !== 'function')
22362247
throw new errors.TypeError('ERR_INVALID_CALLBACK');

‎test/parallel/test-http2-compat-socket.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
// Flags: --expose_internals
2+
13
'use strict';
24

35
const common = require('../common');
@@ -7,6 +9,8 @@ const assert = require('assert');
79
const h2 = require('http2');
810
const net = require('net');
911

12+
const { kTimeout } = require('internal/timers');
13+
1014
// Tests behavior of the proxied socket in Http2ServerRequest
1115
// & Http2ServerResponse - this proxy socket should mimic the
1216
// behavior of http1 but against the http2 api & model
@@ -31,7 +35,7 @@ server.on('request', common.mustCall(function(request, response) {
3135
assert.strictEqual(request.socket.destroyed, false);
3236

3337
request.socket.setTimeout(987);
34-
assert.strictEqual(request.stream.session._idleTimeout, 987);
38+
assert.strictEqual(request.stream.session[kTimeout]._idleTimeout, 987);
3539
request.socket.setTimeout(0);
3640

3741
common.expectsError(() => request.socket.read(), errMsg);

‎test/parallel/test-http2-socket-proxy.js‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
// Flags: --expose_internals
2+
13
'use strict';
24

35
const common = require('../common');
@@ -7,6 +9,8 @@ const assert = require('assert');
79
const h2 = require('http2');
810
const net = require('net');
911

12+
const { kTimeout } = require('internal/timers');
13+
1014
// Tests behavior of the proxied socket on Http2Session
1115

1216
const errMsg = {
@@ -29,7 +33,7 @@ server.on('stream', common.mustCall(function(stream, headers) {
2933
assert.strictEqual(typeof socket.address(), 'object');
3034

3135
socket.setTimeout(987);
32-
assert.strictEqual(session._idleTimeout, 987);
36+
assert.strictEqual(session[kTimeout]._idleTimeout, 987);
3337

3438
common.expectsError(() => socket.destroy, errMsg);
3539
common.expectsError(() => socket.emit, errMsg);
@@ -59,9 +63,6 @@ server.on('stream', common.mustCall(function(stream, headers) {
5963

6064
stream.end();
6165

62-
socket.setTimeout = undefined;
63-
assert.strictEqual(session.setTimeout, undefined);
64-
6566
stream.session.on('close', common.mustCall(() => {
6667
assert.strictEqual(session.socket, undefined);
6768
}));

‎test/parallel/test-http2-timeouts.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ server.on('stream', common.mustCall((stream) => {
2020
{
2121
code: 'ERR_INVALID_ARG_TYPE',
2222
type: TypeError,
23-
message: 'The "msecs" argument must be of type number'
23+
message:
24+
'The "msecs" argument must be of type number. Received type string'
2425
}
2526
);
2627
common.expectsError(

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL