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

http2: emit session close before stream close · nodejs/node@39f61fb · GitHub

/ node Public

Commit 39f61fb

Browse files
mcollina
authored and
sxa
committed
http2: emit session close before stream close
Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #63414 Fixes: #63412 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
1 parent 5a95a2b commit 39f61fb

4 files changed

Lines changed: 110 additions & 26 deletions

File tree

‎doc/api/http2.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1110,10 +1110,12 @@ creates and returns an `Http2Stream` instance that can be used to send an
11101110
HTTP/2 request to the connected server.
11111111

11121112
When a `ClientHttp2Session` is first created, the socket may not yet be
1113-
connected. if `clienthttp2session.request()` is called during this time, the
1113+
connected. If `clienthttp2session.request()` is called during this time, the
11141114
actual request will be deferred until the socket is ready to go.
1115-
If the `session` is closed before the actual request be executed, an
1116-
`ERR_HTTP2_GOAWAY_SESSION` is thrown.
1115+
1116+
If the session becomes unavailable before the request can be created, the
1117+
returned stream will emit `ERR_HTTP2_GOAWAY_SESSION` or
1118+
`ERR_HTTP2_INVALID_SESSION` asynchronously.
11171119

11181120
This method is only available if `http2session.type` is equal to
11191121
`http2.constants.NGHTTP2_SESSION_CLIENT`.

‎lib/internal/http2/core.js‎

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -845,6 +845,10 @@ function requestOnConnect(headersList, options) {
845845
}
846846
}
847847

848+
function requestOnError(error) {
849+
this.destroy(error);
850+
}
851+
848852
// Validates that priority options are correct, specifically:
849853
// 1. options.weight must be a number
850854
// 2. options.parent must be a positive number
@@ -1160,7 +1164,7 @@ function setupHandle(socket, type, options) {
11601164
process.nextTick(emit, this, 'connect', this, socket);
11611165
}
11621166

1163-
// Emits a close event followed by an error event if err is truthy. Used
1167+
// Emits an error event followed by a close event if err is truthy. Used
11641168
// by Http2Session.prototype.destroy()
11651169
function emitClose(self, error) {
11661170
if (error)
@@ -1231,17 +1235,16 @@ function closeSession(session, code, error) {
12311235
session.setTimeout(0);
12321236
session.removeAllListeners('timeout');
12331237

1238+
const socket = session[kSocket];
1239+
const handle = session[kHandle];
1240+
12341241
// Destroy any pending and open streams
12351242
if (state.pendingStreams.size > 0 || state.streams.size > 0) {
12361243
const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
12371244
state.pendingStreams.forEach((stream) => stream.destroy(cancel));
12381245
state.streams.forEach((stream) => stream.destroy(error));
12391246
}
12401247

1241-
// Disassociate from the socket and server.
1242-
const socket = session[kSocket];
1243-
const handle = session[kHandle];
1244-
12451248
// Destroy the handle if it exists at this point.
12461249
if (handle !== undefined) {
12471250
handle.ondone = finishSessionClose.bind(null, session, error);
@@ -1816,11 +1819,15 @@ class ClientHttp2Session extends Http2Session {
18161819
request(headersParam, options) {
18171820
debugSessionObj(this, 'initiating request');
18181821

1819-
if (this.destroyed)
1820-
throw new ERR_HTTP2_INVALID_SESSION();
1821-
1822-
if (this.closed)
1823-
throw new ERR_HTTP2_GOAWAY_SESSION();
1822+
// Keep argument validation synchronous, but defer session-state failures
1823+
// to the returned stream so request retries from stream callbacks do not
1824+
// throw before session lifecycle handlers run.
1825+
let requestError;
1826+
if (this.destroyed) {
1827+
requestError = new ERR_HTTP2_INVALID_SESSION();
1828+
} else if (this.closed) {
1829+
requestError = new ERR_HTTP2_GOAWAY_SESSION();
1830+
}
18241831

18251832
this[kUpdateTimer]();
18261833

@@ -1906,19 +1913,24 @@ class ClientHttp2Session extends Http2Session {
19061913
}
19071914
}
19081915

1909-
const onConnect = reqAsync.bind(requestOnConnect.bind(stream, headersList, options));
1910-
if (this.connecting) {
1911-
if (this[kPendingRequestCalls] !== null) {
1912-
this[kPendingRequestCalls].push(onConnect);
1916+
if (requestError) {
1917+
process.nextTick(reqAsync.bind(requestOnError.bind(stream, requestError)));
1918+
} else {
1919+
const onConnect = reqAsync.bind(
1920+
requestOnConnect.bind(stream, headersList, options));
1921+
if (this.connecting) {
1922+
if (this[kPendingRequestCalls] !== null) {
1923+
this[kPendingRequestCalls].push(onConnect);
1924+
} else {
1925+
this[kPendingRequestCalls] = [onConnect];
1926+
this.once('connect', () => {
1927+
this[kPendingRequestCalls].forEach((f) => f());
1928+
this[kPendingRequestCalls] = null;
1929+
});
1930+
}
19131931
} else {
1914-
this[kPendingRequestCalls] = [onConnect];
1915-
this.once('connect', () => {
1916-
this[kPendingRequestCalls].forEach((f) => f());
1917-
this[kPendingRequestCalls] = null;
1918-
});
1932+
onConnect();
19191933
}
1920-
} else {
1921-
onConnect();
19221934
}
19231935

19241936
if (onClientStreamCreatedChannel.hasSubscribers) {

‎test/parallel/test-http2-client-destroy.js‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,19 @@ const { listenerCount } = require('events');
8181
assert.throws(() => client.ping(), sessionError);
8282
assert.throws(() => client.settings({}), sessionError);
8383
assert.throws(() => client.goaway(), sessionError);
84-
assert.throws(() => client.request(), sessionError);
84+
85+
const pendingReq = client.request();
86+
pendingReq.on('response', common.mustNotCall());
87+
pendingReq.on('error', common.expectsError(sessionError));
88+
pendingReq.on('close', common.mustCall());
89+
90+
client.on('close', common.mustCall(() => {
91+
const postCloseReq = client.request();
92+
postCloseReq.on('response', common.mustNotCall());
93+
postCloseReq.on('error', common.expectsError(sessionError));
94+
postCloseReq.on('close', common.mustCall());
95+
}));
96+
8597
client.close(); // Should be a non-op at this point
8698

8799
// Wait for setImmediate call from destroy() to complete
@@ -92,7 +104,6 @@ const { listenerCount } = require('events');
92104
assert.throws(() => client.ping(), sessionError);
93105
assert.throws(() => client.settings({}), sessionError);
94106
assert.throws(() => client.goaway(), sessionError);
95-
assert.throws(() => client.request(), sessionError);
96107
client.close(); // Should be a non-op at this point
97108
}));
98109

‎test/parallel/test-http2-client-session-close-before-stream-close.js‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const assert = require('assert');
8+
const http2 = require('http2');
9+
10+
const server = http2.createServer();
11+
let serverSocket;
12+
13+
server.on('connection', common.mustCall((socket) => {
14+
serverSocket = socket;
15+
socket.on('error', () => {});
16+
}));
17+
18+
server.on('sessionError', () => {});
19+
server.on('stream', common.mustCall((stream, headers) => {
20+
if (headers[':path'] === '/close') {
21+
stream.respond({ ':status': 200 });
22+
stream.write('partial', common.mustCall(() => {
23+
setImmediate(() => serverSocket.destroy());
24+
}));
25+
return;
26+
}
27+
28+
stream.respond({ ':status': 200 });
29+
stream.end('ok');
30+
}));
31+
32+
server.listen(0, common.mustCall(() => {
33+
const session = http2.connect(`http://localhost:${server.address().port}`);
34+
let cachedSession = session;
35+
36+
session.on('error', () => {});
37+
session.on('close', common.mustCall(() => {
38+
cachedSession = undefined;
39+
server.close();
40+
}));
41+
42+
const req = session.request({ ':path': '/close' });
43+
req.on('response', common.mustCall());
44+
req.on('error', () => {});
45+
req.on('close', common.mustCall(() => {
46+
// This must not throw synchronously even though the session is no longer
47+
// usable. Depending on teardown timing, the returned stream may report a
48+
// closed session before the destroy state is fully observable here.
49+
const req2 = session.request({ ':path': '/again' });
50+
51+
req2.on('error', common.mustCall((err) => {
52+
assert.ok(
53+
err.code === 'ERR_HTTP2_INVALID_SESSION' ||
54+
err.code === 'ERR_HTTP2_GOAWAY_SESSION');
55+
assert.strictEqual(cachedSession, undefined);
56+
}));
57+
}));
58+
req.resume();
59+
}));

0 commit comments

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