From 4d7aba28e7af8114df8e079d3975c3607e14a6b1 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 10 Oct 2024 23:03:39 +0200 Subject: [PATCH 01/52] http2, tls: check content-length, fix RST and GOAWAY logic From 60bdd9f50bd01ffdc918aa6b9bb84ec0d8e021e9 Mon Sep 17 00:00:00 2001 From: Gil Pedersen Date: Tue, 15 Sep 2020 12:36:34 +0000 Subject: [PATCH 02/52] Check content-length before calling stream_close_callback with error code 0 --- deps/nghttp2/lib/nghttp2_session.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index 54746fb37b376d..08efaf0f8e9bda 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -1485,6 +1485,11 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id, DEBUGF("stream: stream(%p)=%d close\n", stream, stream->stream_id); + if (error_code == NGHTTP2_NO_ERROR && + nghttp2_http_on_remote_end_stream(stream) == -1) { + error_code = NGHTTP2_PROTOCOL_ERROR; + } + /* We call on_stream_close_callback even if stream->state is NGHTTP2_STREAM_INITIAL. This will happen while sending request HEADERS, a local endpoint receives RST_STREAM for that stream. It From 0ee22c8aec91cc90e92c51a3fa24090260a46346 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 26 May 2024 07:02:30 +0200 Subject: [PATCH 03/52] fix(http2): check content-length, fix sending RST --- .../nghttp2/patches/0000-content-length.patch | 29 ++++++++++++++++++ lib/internal/http2/core.js | 30 +++++++++++++++---- src/node_http2.cc | 5 ++++ test/parallel/test-http2-client-destroy.js | 30 +++++++++++++++++-- .../test-http2-client-jsstream-destroy.js | 6 ++++ .../test-http2-client-socket-destroy.js | 7 ++++- ...est-http2-compat-serverresponse-destroy.js | 8 +++-- ...at-serverresponse-headers-after-destroy.js | 8 ++++- ...est-http2-compat-socket-destroy-delayed.js | 8 ++++- test/parallel/test-http2-compat-socket-set.js | 4 ++- test/parallel/test-http2-compat-socket.js | 3 +- .../test-http2-compat-write-head-destroyed.js | 5 ++++ .../test-http2-destroy-after-write.js | 5 ++++ .../test-http2-large-write-destroy.js | 5 ++++ test/parallel/test-http2-respond-errors.js | 4 ++- .../test-http2-respond-file-errors.js | 1 + .../test-http2-respond-file-fd-errors.js | 1 + ...ttp2-respond-with-file-connection-abort.js | 5 ++++ ...st-http2-server-shutdown-options-errors.js | 1 + ...est-http2-server-stream-session-destroy.js | 7 ++++- .../parallel/test-http2-zero-length-header.js | 17 +++++++---- .../test-http2-max-session-memory.js | 5 +--- 22 files changed, 167 insertions(+), 27 deletions(-) create mode 100644 deps/nghttp2/patches/0000-content-length.patch diff --git a/deps/nghttp2/patches/0000-content-length.patch b/deps/nghttp2/patches/0000-content-length.patch new file mode 100644 index 00000000000000..c898499e37d6ca --- /dev/null +++ b/deps/nghttp2/patches/0000-content-length.patch @@ -0,0 +1,29 @@ +From 92c120c0ba2ed5806960f8ec9a338fefc17b0427 Mon Sep 17 00:00:00 2001 +From: Gil Pedersen +Date: Tue, 15 Sep 2020 12:36:34 +0000 +Subject: [PATCH] Check content-length before calling stream_close_callback + with error code 0 + +--- + deps/nghttp2/lib/nghttp2_session.c | 5 +++++ + 1 file changed, 5 insertions(+) + +diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c +index 004a4dff..008f9d34 100644 +--- a/deps/nghttp2/lib/nghttp2_session.c ++++ b/deps/nghttp2/lib/nghttp2_session.c +@@ -1485,6 +1485,11 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id, + + DEBUGF("stream: stream(%p)=%d close\n", stream, stream->stream_id); + ++ if (error_code == NGHTTP2_NO_ERROR && ++ nghttp2_http_on_remote_end_stream(stream) == -1) { ++ error_code = NGHTTP2_PROTOCOL_ERROR; ++ } ++ + /* We call on_stream_close_callback even if stream->state is + NGHTTP2_STREAM_INITIAL. This will happen while sending request + HEADERS, a local endpoint receives RST_STREAM for that stream. It +-- +2.40.1 + diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 923db9ad1b5b1e..3ddb54dd174212 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2312,6 +2312,9 @@ class Http2Stream extends Duplex { const state = this[kState]; const sessionState = session[kState]; + const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; + + const defaultCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; const sessionCode = sessionState.goawayCode || sessionState.destroyCode; // If a stream has already closed successfully, there is no error @@ -2334,8 +2337,12 @@ class Http2Stream extends Duplex { } const hasHandle = handle !== undefined; - if (!this.closed) - closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream); + if (!this.closed) { + const rstStreamStatus = hasHandle ? kForceRstStream : kNoRstStream; + + closeStream(this, code || defaultCode, rstStreamStatus); + } + this.push(null); if (hasHandle) { @@ -2349,11 +2356,21 @@ class Http2Stream extends Duplex { sessionState.writeQueueSize -= state.writeQueueSize; state.writeQueueSize = 0; + // The client should not throw when it sends CANCEL to the server. + // The client should throw when it receives CANCEL from the server. + // The server should not throw on CANCEL under any circumstances. + // The first check is not sufficient as it doesn't cover cases + // with connection resets like test-http2-max-session-memory.js + const throwOnCancel = !this.aborted && !isServerSession; + // RST code 8 not emitted as an error as its used by clients to signify // abort and is already covered by aborted event, also allows more // seamless compatibility with http1 - if (err == null && code !== NGHTTP2_NO_ERROR && code !== NGHTTP2_CANCEL) + if (err == null + && code !== NGHTTP2_NO_ERROR + && (code !== NGHTTP2_CANCEL || throwOnCancel)) { err = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); + } this[kSession] = undefined; this[kHandle] = undefined; @@ -3279,8 +3296,11 @@ function socketOnClose() { debugSessionObj(session, 'socket closed'); const err = session.connecting ? new ERR_SOCKET_CLOSED() : null; const state = session[kState]; - state.streams.forEach((stream) => stream.close(NGHTTP2_CANCEL)); - state.pendingStreams.forEach((stream) => stream.close(NGHTTP2_CANCEL)); + const code = session[kType] === NGHTTP2_SESSION_SERVER + ? NGHTTP2_CANCEL + : NGHTTP2_INTERNAL_ERROR; + state.streams.forEach((stream) => stream.close(code)); + state.pendingStreams.forEach((stream) => stream.close(code)); session.close(); session[kMaybeDestroy](err); } diff --git a/src/node_http2.cc b/src/node_http2.cc index f4f85b37781817..be36d2fa6930e7 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2656,6 +2656,11 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, if (stream->available_outbound_length_ == 0 && !stream->is_writable()) { Debug(session, "no more data for stream %d", id); *flags |= NGHTTP2_DATA_FLAG_EOF; + + if (session->has_pending_rststream(id)) { + *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; + } + if (stream->has_trailers()) { *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; stream->OnTrailers(); diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index d7609fc33391ac..bcc3184fbffbec 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -102,7 +102,7 @@ const { getEventListeners } = require('events'); })); } -// Test destroy before goaway +// Test server session destroy { const server = h2.createServer(); server.on('stream', common.mustCall((stream) => { @@ -114,8 +114,32 @@ const { getEventListeners } = require('events'); client.on('close', () => { server.close(); - // Calling destroy in here should not matter - client.destroy(); + }); + + const req = client.request(); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + })); +} + +// Test client session destroy +{ + let client; + const server = h2.createServer(); + server.on('stream', common.mustCall((stream) => { + stream.on('error', common.mustNotCall()); + stream.once('aborted', common.mustCall()); + client.destroy(); + })); + + server.listen(0, common.mustCall(() => { + client = h2.connect(`http://localhost:${server.address().port}`); + + client.on('close', () => { + server.close(); }); client.request(); diff --git a/test/parallel/test-http2-client-jsstream-destroy.js b/test/parallel/test-http2-client-jsstream-destroy.js index 7e44241e985a64..ebca22737ce953 100644 --- a/test/parallel/test-http2-client-jsstream-destroy.js +++ b/test/parallel/test-http2-client-jsstream-destroy.js @@ -50,6 +50,12 @@ server.listen(0, common.mustCall(function() { socket.destroy(); }); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + req.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-client-socket-destroy.js b/test/parallel/test-http2-client-socket-destroy.js index 2cc6ef1e4ea4a8..52cccad787e563 100644 --- a/test/parallel/test-http2-client-socket-destroy.js +++ b/test/parallel/test-http2-client-socket-destroy.js @@ -32,8 +32,13 @@ server.listen(0, common.mustCall(function() { })); req.resume(); - req.on('end', common.mustCall()); + req.on('end', common.mustNotCall()); req.on('close', common.mustCall(() => server.close())); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); // On the client, the close event must call client.on('close', common.mustCall()); diff --git a/test/parallel/test-http2-compat-serverresponse-destroy.js b/test/parallel/test-http2-compat-serverresponse-destroy.js index 1154b69df41548..5d5f78e49b40b7 100644 --- a/test/parallel/test-http2-compat-serverresponse-destroy.js +++ b/test/parallel/test-http2-compat-serverresponse-destroy.js @@ -44,8 +44,12 @@ server.listen(0, common.mustCall(() => { { const req = client.request(); req.on('response', common.mustNotCall()); - req.on('error', common.mustNotCall()); - req.on('end', common.mustCall()); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + req.on('end', common.mustNotCall()); req.on('close', common.mustCall(() => countdown.dec())); req.resume(); } diff --git a/test/parallel/test-http2-compat-serverresponse-headers-after-destroy.js b/test/parallel/test-http2-compat-serverresponse-headers-after-destroy.js index fc97a70f42d956..20d065b9485465 100644 --- a/test/parallel/test-http2-compat-serverresponse-headers-after-destroy.js +++ b/test/parallel/test-http2-compat-serverresponse-headers-after-destroy.js @@ -38,9 +38,15 @@ server.listen(0, common.mustCall(function() { ':authority': `localhost:${port}` }; const request = client.request(headers); - request.on('end', common.mustCall(function() { + request.once('error', common.mustCall(function(error) { + common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })(error); client.close(); })); + request.on('end', common.mustNotCall()); request.end(); request.resume(); })); diff --git a/test/parallel/test-http2-compat-socket-destroy-delayed.js b/test/parallel/test-http2-compat-socket-destroy-delayed.js index 62405047d8266e..6c8174c840fe52 100644 --- a/test/parallel/test-http2-compat-socket-destroy-delayed.js +++ b/test/parallel/test-http2-compat-socket-destroy-delayed.js @@ -32,7 +32,13 @@ app.listen(0, mustCall(() => { request.once('response', mustCall((headers, flags) => { let data = ''; request.on('data', (chunk) => { data += chunk; }); - request.on('end', mustCall(() => { + request.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + request.on('end', common.mustNotCall()); + request.on('close', mustCall(() => { assert.strictEqual(data, 'hello'); session.close(); app.close(); diff --git a/test/parallel/test-http2-compat-socket-set.js b/test/parallel/test-http2-compat-socket-set.js index e8b804858d65bd..34edc4c5459e83 100644 --- a/test/parallel/test-http2-compat-socket-set.js +++ b/test/parallel/test-http2-compat-socket-set.js @@ -87,7 +87,9 @@ server.listen(0, common.mustCall(function() { ':authority': `localhost:${port}` }; const request = client.request(headers); - request.on('end', common.mustCall(() => { + request.on('error', common.mustCall()); + request.on('end', common.mustNotCall()); + request.on('close', common.mustCall(() => { client.close(); server.close(); })); diff --git a/test/parallel/test-http2-compat-socket.js b/test/parallel/test-http2-compat-socket.js index 95bc42180ef8e9..1246f4347691a0 100644 --- a/test/parallel/test-http2-compat-socket.js +++ b/test/parallel/test-http2-compat-socket.js @@ -84,9 +84,10 @@ server.listen(0, common.mustCall(function() { ':authority': `localhost:${port}` }; const request = client.request(headers); - request.on('end', common.mustCall(() => { + request.once('error', common.mustCall(() => { client.close(); })); + request.on('end', common.mustNotCall()); request.end(); request.resume(); })); diff --git a/test/parallel/test-http2-compat-write-head-destroyed.js b/test/parallel/test-http2-compat-write-head-destroyed.js index 842bf0e9abffb0..40ae686a36fd18 100644 --- a/test/parallel/test-http2-compat-write-head-destroyed.js +++ b/test/parallel/test-http2-compat-write-head-destroyed.js @@ -20,6 +20,11 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.on('response', common.mustNotCall()); req.on('close', common.mustCall((arg) => { client.close(); diff --git a/test/parallel/test-http2-destroy-after-write.js b/test/parallel/test-http2-destroy-after-write.js index 399df015b8879e..d1b6b57da86e41 100644 --- a/test/parallel/test-http2-destroy-after-write.js +++ b/test/parallel/test-http2-destroy-after-write.js @@ -25,6 +25,11 @@ server.on('session', common.mustCall(function(session) { server.listen(0, function() { const client = http2.connect(`http://localhost:${server.address().port}`); const stream = client.request({ ':method': 'POST' }); + stream.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); stream.on('response', common.mustCall(function(headers) { assert.strictEqual(headers[':status'], 200); })); diff --git a/test/parallel/test-http2-large-write-destroy.js b/test/parallel/test-http2-large-write-destroy.js index b59c66bb04755b..6a0bfdf8f49efe 100644 --- a/test/parallel/test-http2-large-write-destroy.js +++ b/test/parallel/test-http2-large-write-destroy.js @@ -31,6 +31,11 @@ server.listen(0, common.mustCall(() => { { rejectUnauthorized: false }); const req = client.request({ ':path': '/' }); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.end(); req.resume(); // Otherwise close won't be emitted if there's pending data. diff --git a/test/parallel/test-http2-respond-errors.js b/test/parallel/test-http2-respond-errors.js index cc733b6994a3bd..251e2a622e69b0 100644 --- a/test/parallel/test-http2-respond-errors.js +++ b/test/parallel/test-http2-respond-errors.js @@ -43,7 +43,9 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); - req.on('end', common.mustCall(() => { + req.once('error', common.mustCall()); + req.on('end', common.mustNotCall()); + req.on('close', common.mustCall(() => { client.close(); server.close(); })); diff --git a/test/parallel/test-http2-respond-file-errors.js b/test/parallel/test-http2-respond-file-errors.js index 5c3424f2bc3484..fda59275e43e98 100644 --- a/test/parallel/test-http2-respond-file-errors.js +++ b/test/parallel/test-http2-respond-file-errors.js @@ -94,6 +94,7 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); + req.once('error', common.mustCall()); req.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-respond-file-fd-errors.js b/test/parallel/test-http2-respond-file-fd-errors.js index 5f7e57ea4e45d2..67404c3350dd38 100644 --- a/test/parallel/test-http2-respond-file-fd-errors.js +++ b/test/parallel/test-http2-respond-file-fd-errors.js @@ -117,6 +117,7 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); + req.once('error', common.mustCall()); req.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index d5ed3645708d95..c2ad4e11a503a1 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/test/parallel/test-http2-respond-with-file-connection-abort.js @@ -23,6 +23,11 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.on('response', common.mustCall()); req.once('data', common.mustCall(() => { net.Socket.prototype.destroy.call(client.socket); diff --git a/test/parallel/test-http2-server-shutdown-options-errors.js b/test/parallel/test-http2-server-shutdown-options-errors.js index 5a2ca62a6c8e31..e1c426e2823e75 100644 --- a/test/parallel/test-http2-server-shutdown-options-errors.js +++ b/test/parallel/test-http2-server-shutdown-options-errors.js @@ -59,6 +59,7 @@ server.listen( const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); req.resume(); + req.once('error', common.mustCall()); req.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js index 34b22fdfbd1334..a03c4fcfd21158 100644 --- a/test/parallel/test-http2-server-stream-session-destroy.js +++ b/test/parallel/test-http2-server-stream-session-destroy.js @@ -48,6 +48,11 @@ server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`); const req = client.request(); req.resume(); - req.on('end', common.mustCall()); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + req.on('end', common.mustNotCall()); req.on('close', common.mustCall(() => server.close(common.mustCall()))); })); diff --git a/test/parallel/test-http2-zero-length-header.js b/test/parallel/test-http2-zero-length-header.js index 2e7876858aaace..c48dc202b8f831 100644 --- a/test/parallel/test-http2-zero-length-header.js +++ b/test/parallel/test-http2-zero-length-header.js @@ -7,7 +7,7 @@ const assert = require('assert'); const http2 = require('http2'); const server = http2.createServer(); -server.on('stream', (stream, headers) => { +server.on('stream', common.mustCall((stream, headers) => { assert.deepStrictEqual(headers, { ':scheme': 'http', ':authority': `localhost:${server.address().port}`, @@ -17,10 +17,15 @@ server.on('stream', (stream, headers) => { '__proto__': null, [http2.sensitiveHeaders]: [] }); - stream.session.destroy(); + stream.respond(); + stream.end(); server.close(); -}); -server.listen(0, common.mustCall(() => { - const client = http2.connect(`http://localhost:${server.address().port}/`); - client.request({ ':path': '/', '': 'foo', 'bar': '' }).end(); })); +server.listen(0, () => { + const client = http2.connect(`http://localhost:${server.address().port}/`); + const req = client.request({ ':path': '/', '': 'foo', 'bar': '' }); + req.end(); + req.on('close', () => { + client.close(); + }); +}); diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js index 9b77a45c3227fd..65566d8d31764f 100644 --- a/test/sequential/test-http2-max-session-memory.js +++ b/test/sequential/test-http2-max-session-memory.js @@ -13,10 +13,7 @@ const largeBuffer = Buffer.alloc(2e6); const server = http2.createServer({ maxSessionMemory: 1 }); server.on('stream', common.mustCall((stream) => { - stream.on('error', (err) => { - if (err.code !== 'ECONNRESET') - throw err; - }); + stream.on('error', common.mustNotCall()); stream.respond(); stream.end(largeBuffer); })); From ca3c23de443263fa5527d6df28a4501b7d0bf76f Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 26 May 2024 07:28:17 +0200 Subject: [PATCH 04/52] fix a test --- doc/api/http2.md | 2 +- test/parallel/test-http2-server-socket-destroy.js | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 0bb059fc81d33b..eec71acbab3690 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -507,7 +507,7 @@ added: v8.4.0 due to an error. * `code` {number} The HTTP/2 error code to send in the final `GOAWAY` frame. If unspecified, and `error` is not undefined, the default is `INTERNAL_ERROR`, - otherwise defaults to `NO_ERROR`. + otherwise defaults to `CANCEL`. Immediately terminates the `Http2Session` and the associated `net.Socket` or `tls.TLSSocket`. diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index 4e9a3f5ab375c0..5c4bad464b89c0 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -49,13 +49,11 @@ server.on('listening', common.mustCall(async () => { client.on('close', common.mustCall()); const req = client.request({ ':method': 'POST' }); - // The client may have an ECONNRESET error here depending on the operating - // system, due mainly to differences in the timing of socket closing. Do - // not wrap this in a common mustCall. - req.on('error', (err) => { - if (err.code !== 'ECONNRESET') - throw err; - }); + req.on('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.on('aborted', common.mustCall()); req.resume(); From 2cd1b95218a95fb2a3edc5a3e7c2c43ba1539d9f Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 26 May 2024 12:18:24 +0200 Subject: [PATCH 05/52] [skip ci] fix: lint --- lib/internal/http2/core.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 3ddb54dd174212..11e2cdb851fa9a 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2366,9 +2366,9 @@ class Http2Stream extends Duplex { // RST code 8 not emitted as an error as its used by clients to signify // abort and is already covered by aborted event, also allows more // seamless compatibility with http1 - if (err == null - && code !== NGHTTP2_NO_ERROR - && (code !== NGHTTP2_CANCEL || throwOnCancel)) { + if (err == null && + code !== NGHTTP2_NO_ERROR && + (code !== NGHTTP2_CANCEL || throwOnCancel)) { err = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); } @@ -3296,9 +3296,9 @@ function socketOnClose() { debugSessionObj(session, 'socket closed'); const err = session.connecting ? new ERR_SOCKET_CLOSED() : null; const state = session[kState]; - const code = session[kType] === NGHTTP2_SESSION_SERVER - ? NGHTTP2_CANCEL - : NGHTTP2_INTERNAL_ERROR; + const code = session[kType] === NGHTTP2_SESSION_SERVER ? + NGHTTP2_CANCEL : + NGHTTP2_INTERNAL_ERROR; state.streams.forEach((stream) => stream.close(code)); state.pendingStreams.forEach((stream) => stream.close(code)); session.close(); From 12093cba1f281140731048fc4032659c71e507ce Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 26 May 2024 12:45:20 +0200 Subject: [PATCH 06/52] [skip ci] fix 1 flaky test, 3 more to go --- .../test-http2-server-socket-destroy.js | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index 5c4bad464b89c0..c2addd8c9bdd8b 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -32,27 +32,33 @@ function onStream(stream) { stream.on('aborted', common.mustCall()); assert.notStrictEqual(stream.session, undefined); - socket.destroy(); + + // With destroy(): + // The client may have an ECONNRESET or NGHTTP2_INTERNAL_ERROR error + // depending on the operating system, due mainly to differences + // in the timing of socket closing. The test would be flaky. + + // With resetAndDestroy(): + // TCP is always reset, no flakiness. + socket.resetAndDestroy(); } server.listen(0); server.on('listening', common.mustCall(async () => { const client = h2.connect(`http://localhost:${server.address().port}`); - // The client may have an ECONNRESET error here depending on the operating - // system, due mainly to differences in the timing of socket closing. Do - // not wrap this in a common mustCall. - client.on('error', (err) => { - if (err.code !== 'ECONNRESET') - throw err; - }); + client.on('error', common.expectsError({ + name: 'Error', + code: 'ECONNRESET', + message: 'read ECONNRESET' + })); client.on('close', common.mustCall()); const req = client.request({ ':method': 'POST' }); req.on('error', common.expectsError({ name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + code: 'ECONNRESET', + message: 'read ECONNRESET' })); req.on('aborted', common.mustCall()); From c3630857dc3f8cedcf4d471be446e36a23a9a016 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 26 May 2024 18:35:05 +0200 Subject: [PATCH 07/52] more bug fixes --- lib/internal/http2/core.js | 39 +++++++++------- test/parallel/test-http2-client-destroy.js | 44 +++++++++++++++++-- test/parallel/test-http2-compat-aborted.js | 2 +- test/parallel/test-http2-compat-errors.js | 2 +- .../test-http2-compat-serverresponse-close.js | 2 +- ...esponse-end-after-statuses-without-body.js | 2 +- .../test-http2-compat-serverresponse-write.js | 13 ++++-- .../test-http2-options-server-request.js | 2 +- test/parallel/test-http2-server-errors.js | 10 +++++ .../test-http2-server-session-destroy.js | 12 +++++ ...st-http2-server-shutdown-options-errors.js | 6 +++ ...est-http2-server-stream-session-destroy.js | 10 +++-- .../test-http2-max-session-memory.js | 5 --- 13 files changed, 113 insertions(+), 36 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 11e2cdb851fa9a..23197212c4b577 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1236,7 +1236,7 @@ class Http2Session extends EventEmitter { socket.on('close', socketOnClose); this[kState] = { - destroyCode: NGHTTP2_NO_ERROR, + destroyCode: null, flags: SESSION_FLAGS_PENDING, goawayCode: null, goawayLastStreamID: null, @@ -1436,7 +1436,7 @@ class Http2Session extends EventEmitter { // If a GOAWAY frame has been received, gives the error code specified get goawayCode() { - return this[kState].goawayCode || NGHTTP2_NO_ERROR; + return this[kState].goawayCode; } // If a GOAWAY frame has been received, gives the last stream ID reported @@ -1981,7 +1981,7 @@ class Http2Stream extends Duplex { this[kState] = { didRead: false, flags: STREAM_FLAGS_PENDING, - rstCode: NGHTTP2_NO_ERROR, + rstCode: undefined, writeQueueSize: 0, trailersReady: false, endAfterHeaders: false, @@ -2314,8 +2314,16 @@ class Http2Stream extends Duplex { const sessionState = session[kState]; const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; - const defaultCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; - const sessionCode = sessionState.goawayCode || sessionState.destroyCode; + let sessionCode; + if (sessionState.goawayCode === NGHTTP2_NO_ERROR) { + // Received NO_ERROR (GOAWAY) from remote peer + sessionCode = NGHTTP2_INTERNAL_ERROR; + } else if (sessionState.destroyCode === NGHTTP2_NO_ERROR) { + // Code to send to remote peer + sessionCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; + } else { + sessionCode = sessionState.goawayCode ?? sessionState.destroyCode; + } // If a stream has already closed successfully, there is no error // to report from this stream, even if the session has errored. @@ -2324,11 +2332,9 @@ class Http2Stream extends Duplex { // this stream's close and destroy operations. // Previously, this always overrode a successful close operation code // NGHTTP2_NO_ERROR (0) with sessionCode because the use of the || operator. - let code = this.closed ? this.rstCode : sessionCode; - if (err != null) { - if (sessionCode) { - code = sessionCode; - } else if (err instanceof AbortError) { + let code = this.rstCode ?? sessionCode; + if (err != null && code === null) { + if (err instanceof AbortError) { // Enables using AbortController to cancel requests with RST code 8. code = NGHTTP2_CANCEL; } else { @@ -2340,7 +2346,8 @@ class Http2Stream extends Duplex { if (!this.closed) { const rstStreamStatus = hasHandle ? kForceRstStream : kNoRstStream; - closeStream(this, code || defaultCode, rstStreamStatus); + const defaultCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; + closeStream(this, code ?? defaultCode, rstStreamStatus); } this.push(null); @@ -2356,16 +2363,16 @@ class Http2Stream extends Duplex { sessionState.writeQueueSize -= state.writeQueueSize; state.writeQueueSize = 0; - // The client should not throw when it sends CANCEL to the server. + // The client should not throw when it sends CANCEL to the server + // before ending the request. + // The client should throw when it sends CANCEL to the server + // after ending the request. // The client should throw when it receives CANCEL from the server. - // The server should not throw on CANCEL under any circumstances. + // The server should not throw when it receives CANCEL from the client. // The first check is not sufficient as it doesn't cover cases // with connection resets like test-http2-max-session-memory.js const throwOnCancel = !this.aborted && !isServerSession; - // RST code 8 not emitted as an error as its used by clients to signify - // abort and is already covered by aborted event, also allows more - // seamless compatibility with http1 if (err == null && code !== NGHTTP2_NO_ERROR && (code !== NGHTTP2_CANCEL || throwOnCancel)) { diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index bcc3184fbffbec..f4a103d7f389a9 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -106,6 +106,11 @@ const { getEventListeners } = require('events'); { const server = h2.createServer(); server.on('stream', common.mustCall((stream) => { + stream.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); stream.session.destroy(); })); @@ -130,7 +135,11 @@ const { getEventListeners } = require('events'); let client; const server = h2.createServer(); server.on('stream', common.mustCall((stream) => { - stream.on('error', common.mustNotCall()); + // FIXME: race condition with ECONNRESET? + // It receives GOAWAY from the client, + // so it throws with NGHTTP2_INTERNAL_ERROR instead. + // At least on macOS. + stream.on('error', common.mustCall()); stream.once('aborted', common.mustCall()); client.destroy(); })); @@ -142,11 +151,16 @@ const { getEventListeners } = require('events'); server.close(); }); - client.request(); + const req = client.request(); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); })); } -// Test destroy before connect +// Test destroy before connect (endStream) { const server = h2.createServer(); server.on('stream', common.mustNotCall()); @@ -160,6 +174,30 @@ const { getEventListeners } = require('events'); })); const req = client.request(); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); + req.destroy(); + })); +} + +// Test destroy before connect (no endStream) +{ + const server = h2.createServer(); + server.on('stream', common.mustNotCall()); + + server.listen(0, common.mustCall(() => { + const client = h2.connect(`http://localhost:${server.address().port}`); + + server.on('connection', common.mustCall(() => { + server.close(); + client.close(); + })); + + const req = client.request({}, { endStream: false }); + req.on('error', common.mustNotCall()); req.destroy(); })); } diff --git a/test/parallel/test-http2-compat-aborted.js b/test/parallel/test-http2-compat-aborted.js index 0ed0d800435f38..5c525b4d3171db 100644 --- a/test/parallel/test-http2-compat-aborted.js +++ b/test/parallel/test-http2-compat-aborted.js @@ -21,7 +21,7 @@ const server = h2.createServer(common.mustCall(function(req, res) { server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request(); + const request = client.request({}, { endStream: false }); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-errors.js b/test/parallel/test-http2-compat-errors.js index 18dc385422a48e..2dbea121c962b1 100644 --- a/test/parallel/test-http2-compat-errors.js +++ b/test/parallel/test-http2-compat-errors.js @@ -27,7 +27,7 @@ const server = h2.createServer(common.mustCall(function(req, res) { server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request(); + const request = client.request({}, { endStream: false }); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-close.js b/test/parallel/test-http2-compat-serverresponse-close.js index 71079f425c97fc..18922bee4d94a9 100644 --- a/test/parallel/test-http2-compat-serverresponse-close.js +++ b/test/parallel/test-http2-compat-serverresponse-close.js @@ -22,7 +22,7 @@ server.listen(0); server.on('listening', () => { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request(); + const request = client.request({}, { endStream: false }); request.on('data', common.mustCall(function(chunk) { client.destroy(); server.close(); diff --git a/test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js index ce8cbe600c8bd8..7279273605fc45 100644 --- a/test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js +++ b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js @@ -33,7 +33,7 @@ server.listen(0, common.mustCall(function() { let responseCount = 0; const closeAfterResponse = () => { if (STATUS_CODES_COUNT === ++responseCount) { - client.destroy(); + client.close(); server.close(); } }; diff --git a/test/parallel/test-http2-compat-serverresponse-write.js b/test/parallel/test-http2-compat-serverresponse-write.js index 64b37e8a132e1a..815ad714151b26 100644 --- a/test/parallel/test-http2-compat-serverresponse-write.js +++ b/test/parallel/test-http2-compat-serverresponse-write.js @@ -3,6 +3,7 @@ const { mustCall, mustNotCall, + expectsError, hasCrypto, skip } = require('../common'); @@ -38,7 +39,7 @@ const assert = require('assert'); // response.write() with cb returns falsy value assert(!response.write('muahaha', mustCall())); - client.destroy(); + client.close(); server.close(); })); })); @@ -65,7 +66,7 @@ const assert = require('assert'); response.end(); response.write('asd', mustCall((err) => { assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END'); - client.destroy(); + client.close(); server.close(); })); })); @@ -78,13 +79,17 @@ const assert = require('assert'); const port = server.address().port; const url = `http://localhost:${port}`; const client = connect(url, mustCall(() => { - client.request(); + client.request().once('error', expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); })); server.once('request', mustCall((request, response) => { response.destroy(); assert.strictEqual(response.write('asd', mustNotCall()), false); - client.destroy(); + client.close(); server.close(); })); })); diff --git a/test/parallel/test-http2-options-server-request.js b/test/parallel/test-http2-options-server-request.js index 2143d379823d51..ce84eab9ab4c57 100644 --- a/test/parallel/test-http2-options-server-request.js +++ b/test/parallel/test-http2-options-server-request.js @@ -35,6 +35,6 @@ server.on('listening', common.mustCall(() => { req.resume(); req.on('end', common.mustCall(() => { server.close(); - client.destroy(); + client.close(); })); })); diff --git a/test/parallel/test-http2-server-errors.js b/test/parallel/test-http2-server-errors.js index 959ddccdc7c28a..f9a88307306310 100644 --- a/test/parallel/test-http2-server-errors.js +++ b/test/parallel/test-http2-server-errors.js @@ -38,6 +38,11 @@ const h2 = require('http2'); ':authority': `localhost:${port}`, }; const request = client.request(headers); + request.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); request.on('data', common.mustCall(function(chunk) { // Cause an error on the server side client.destroy(); @@ -78,6 +83,11 @@ const h2 = require('http2'); ':authority': `localhost:${port}`, }; const request = client.request(headers); + request.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); request.on('data', common.mustCall(function(chunk) { // Cause an error on the server side client.destroy(); diff --git a/test/parallel/test-http2-server-session-destroy.js b/test/parallel/test-http2-server-session-destroy.js index afa3dd3398dba8..689edd00a6111c 100644 --- a/test/parallel/test-http2-server-session-destroy.js +++ b/test/parallel/test-http2-server-session-destroy.js @@ -6,11 +6,23 @@ if (!common.hasCrypto) const h2 = require('http2'); const server = h2.createServer(); +server.once('stream', stream => { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); +}); + server.listen(0, common.localhostIPv4, common.mustCall(() => { const afterConnect = common.mustCall((session) => { session.request({ ':method': 'POST' }).end(common.mustCall(() => { session.destroy(); server.close(); + })).once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' })); }); diff --git a/test/parallel/test-http2-server-shutdown-options-errors.js b/test/parallel/test-http2-server-shutdown-options-errors.js index e1c426e2823e75..654a0b0ddc6b49 100644 --- a/test/parallel/test-http2-server-shutdown-options-errors.js +++ b/test/parallel/test-http2-server-shutdown-options-errors.js @@ -17,6 +17,12 @@ const types = [ ]; server.on('stream', common.mustCall((stream) => { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + const session = stream.session; for (const input of types) { diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js index a03c4fcfd21158..14d12a33f686e0 100644 --- a/test/parallel/test-http2-server-stream-session-destroy.js +++ b/test/parallel/test-http2-server-stream-session-destroy.js @@ -34,9 +34,13 @@ server.on('stream', common.mustCall((stream) => { name: 'Error' } ); - // When session is destroyed all streams are destroyed and no further - // error should be emitted. - stream.on('error', common.mustNotCall()); + // When session is forcibly destroyed, + // all streams are destroyed with an error. + stream.on('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); assert.strictEqual(stream.write('data', common.expectsError({ name: 'Error', code: 'ERR_STREAM_WRITE_AFTER_END', diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js index 65566d8d31764f..71194f950a75af 100644 --- a/test/sequential/test-http2-max-session-memory.js +++ b/test/sequential/test-http2-max-session-memory.js @@ -28,11 +28,6 @@ server.listen(0, common.mustCall(() => { // This one should be rejected because the server is over budget // on the current memory allocation const req = client.request(); - req.on('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM' - })); req.on('close', common.mustCall(() => { server.close(); client.destroy(); From 771bf729f3661ce7b63db4392f9deaa27fff1347 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 26 May 2024 18:58:10 +0200 Subject: [PATCH 08/52] [skip ci] fix a test --- test/parallel/test-http2-options-server-response.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http2-options-server-response.js b/test/parallel/test-http2-options-server-response.js index 6f1ae1881d22d8..8a85faa16961eb 100644 --- a/test/parallel/test-http2-options-server-response.js +++ b/test/parallel/test-http2-options-server-response.js @@ -29,6 +29,6 @@ server.on('listening', common.mustCall(() => { req.resume(); req.on('end', common.mustCall(() => { server.close(); - client.destroy(); + client.close(); })); })); From 0ad5ec5bb97c6a2a44cc78fa706c06d949345d96 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Mon, 27 May 2024 01:47:58 +0200 Subject: [PATCH 09/52] more bug fixes --- lib/internal/http2/core.js | 37 ++++++++----------- .../test-http2-cancel-while-client-reading.js | 5 +++ ...test-http2-client-connection-tunnelling.js | 6 +++ test/parallel/test-http2-client-destroy.js | 22 +++++++---- .../test-http2-client-socket-destroy.js | 5 +++ .../test-http2-close-while-writing.js | 10 +++++ test/parallel/test-http2-compat-aborted.js | 5 +++ test/parallel/test-http2-compat-errors.js | 5 +++ .../test-http2-compat-serverresponse-close.js | 5 +++ .../test-http2-destroy-after-write.js | 5 +++ .../test-http2-large-write-destroy.js | 6 +++ .../test-http2-many-writes-and-destroy.js | 5 +++ ...t-http2-multistream-destroy-on-read-tls.js | 11 ++++++ ...test-http2-options-max-reserved-streams.js | 8 +++- test/parallel/test-http2-respond-errors.js | 5 +++ .../test-http2-respond-file-errors.js | 5 +++ .../test-http2-respond-file-fd-errors.js | 6 +++ test/parallel/test-http2-server-rst-stream.js | 2 +- ...tp2-write-finishes-after-stream-destroy.js | 2 + .../test-http2-max-session-memory.js | 8 +++- 20 files changed, 129 insertions(+), 34 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 23197212c4b577..54e013990dcb76 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2314,17 +2314,20 @@ class Http2Stream extends Duplex { const sessionState = session[kState]; const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; - let sessionCode; - if (sessionState.goawayCode === NGHTTP2_NO_ERROR) { - // Received NO_ERROR (GOAWAY) from remote peer - sessionCode = NGHTTP2_INTERNAL_ERROR; - } else if (sessionState.destroyCode === NGHTTP2_NO_ERROR) { + let goawayCode = sessionState.goawayCode; + if (goawayCode === NGHTTP2_NO_ERROR) { + // Received NO_ERROR (GOAWAY) from remote peer yet still got reset + goawayCode = NGHTTP2_INTERNAL_ERROR; + } + + let destroyCode = sessionState.destroyCode; + if (destroyCode === NGHTTP2_NO_ERROR) { // Code to send to remote peer - sessionCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; - } else { - sessionCode = sessionState.goawayCode ?? sessionState.destroyCode; + destroyCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; } + const sessionCode = goawayCode ?? destroyCode; + // If a stream has already closed successfully, there is no error // to report from this stream, even if the session has errored. // This can happen if the stream was already in process of destroying @@ -2347,7 +2350,9 @@ class Http2Stream extends Duplex { const rstStreamStatus = hasHandle ? kForceRstStream : kNoRstStream; const defaultCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; - closeStream(this, code ?? defaultCode, rstStreamStatus); + code ??= defaultCode; + + closeStream(this, code, rstStreamStatus); } this.push(null); @@ -2363,19 +2368,7 @@ class Http2Stream extends Duplex { sessionState.writeQueueSize -= state.writeQueueSize; state.writeQueueSize = 0; - // The client should not throw when it sends CANCEL to the server - // before ending the request. - // The client should throw when it sends CANCEL to the server - // after ending the request. - // The client should throw when it receives CANCEL from the server. - // The server should not throw when it receives CANCEL from the client. - // The first check is not sufficient as it doesn't cover cases - // with connection resets like test-http2-max-session-memory.js - const throwOnCancel = !this.aborted && !isServerSession; - - if (err == null && - code !== NGHTTP2_NO_ERROR && - (code !== NGHTTP2_CANCEL || throwOnCancel)) { + if (err == null && code !== NGHTTP2_NO_ERROR) { err = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); } diff --git a/test/parallel/test-http2-cancel-while-client-reading.js b/test/parallel/test-http2-cancel-while-client-reading.js index 189e128e6667ef..f62d768396a126 100644 --- a/test/parallel/test-http2-cancel-while-client-reading.js +++ b/test/parallel/test-http2-cancel-while-client-reading.js @@ -28,6 +28,11 @@ server.listen(0, function() { { rejectUnauthorized: false } ); client_stream = client.request({ ':method': 'POST' }); + client_stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); client_stream.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-client-connection-tunnelling.js b/test/parallel/test-http2-client-connection-tunnelling.js index 6e04121ca71ea8..cb2887efffc05f 100644 --- a/test/parallel/test-http2-client-connection-tunnelling.js +++ b/test/parallel/test-http2-client-connection-tunnelling.js @@ -63,6 +63,12 @@ netServer.listen(0, common.mustCall(() => { netServer.close(); })); + tlsSocket.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); + // Forcibly kill the TLS socket tlsSocket.destroy(); diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index f4a103d7f389a9..d6de649f1145b6 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -135,11 +135,11 @@ const { getEventListeners } = require('events'); let client; const server = h2.createServer(); server.on('stream', common.mustCall((stream) => { - // FIXME: race condition with ECONNRESET? - // It receives GOAWAY from the client, - // so it throws with NGHTTP2_INTERNAL_ERROR instead. - // At least on macOS. - stream.on('error', common.mustCall()); + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); stream.once('aborted', common.mustCall()); client.destroy(); })); @@ -197,7 +197,11 @@ const { getEventListeners } = require('events'); })); const req = client.request({}, { endStream: false }); - req.on('error', common.mustNotCall()); + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); req.destroy(); })); } @@ -354,7 +358,11 @@ const { getEventListeners } = require('events'); const controller = new AbortController(); server.on('stream', common.mustCall((stream) => { - stream.on('error', common.mustNotCall()); + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); stream.on('close', common.mustCall(() => { assert.strictEqual(stream.rstCode, h2.constants.NGHTTP2_CANCEL); server.close(); diff --git a/test/parallel/test-http2-client-socket-destroy.js b/test/parallel/test-http2-client-socket-destroy.js index 52cccad787e563..795d98e81212f3 100644 --- a/test/parallel/test-http2-client-socket-destroy.js +++ b/test/parallel/test-http2-client-socket-destroy.js @@ -16,6 +16,11 @@ const server = h2.createServer(); // We use the lower-level API here server.on('stream', common.mustCall((stream) => { stream.on('aborted', common.mustCall()); + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); stream.on('close', common.mustCall()); stream.respond(); stream.write(body); diff --git a/test/parallel/test-http2-close-while-writing.js b/test/parallel/test-http2-close-while-writing.js index d8537c31b00eb6..1eddfbaf0af074 100644 --- a/test/parallel/test-http2-close-while-writing.js +++ b/test/parallel/test-http2-close-while-writing.js @@ -23,6 +23,11 @@ let client_stream; server.on('session', common.mustCall(function(session) { session.on('stream', common.mustCall(function(stream) { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); stream.resume(); stream.on('data', function() { this.write(Buffer.alloc(1)); @@ -37,6 +42,11 @@ server.listen(0, function() { maxSessionMemory: 1000 }); client_stream = client.request({ ':method': 'POST' }); + client_stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); client_stream.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-compat-aborted.js b/test/parallel/test-http2-compat-aborted.js index 5c525b4d3171db..3df1f57baf998d 100644 --- a/test/parallel/test-http2-compat-aborted.js +++ b/test/parallel/test-http2-compat-aborted.js @@ -22,6 +22,11 @@ server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { const request = client.request({}, { endStream: false }); + request.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-errors.js b/test/parallel/test-http2-compat-errors.js index 2dbea121c962b1..2d357002366f95 100644 --- a/test/parallel/test-http2-compat-errors.js +++ b/test/parallel/test-http2-compat-errors.js @@ -28,6 +28,11 @@ server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { const request = client.request({}, { endStream: false }); + request.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-close.js b/test/parallel/test-http2-compat-serverresponse-close.js index 18922bee4d94a9..5ba40e5ddf09cc 100644 --- a/test/parallel/test-http2-compat-serverresponse-close.js +++ b/test/parallel/test-http2-compat-serverresponse-close.js @@ -23,6 +23,11 @@ server.on('listening', () => { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { const request = client.request({}, { endStream: false }); + request.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); request.on('data', common.mustCall(function(chunk) { client.destroy(); server.close(); diff --git a/test/parallel/test-http2-destroy-after-write.js b/test/parallel/test-http2-destroy-after-write.js index d1b6b57da86e41..d3324ac7ff340d 100644 --- a/test/parallel/test-http2-destroy-after-write.js +++ b/test/parallel/test-http2-destroy-after-write.js @@ -11,6 +11,11 @@ const server = http2.createServer(); server.on('session', common.mustCall(function(session) { session.on('stream', common.mustCall(function(stream) { + stream.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); stream.on('end', common.mustCall(function() { this.respond({ ':status': 200 diff --git a/test/parallel/test-http2-large-write-destroy.js b/test/parallel/test-http2-large-write-destroy.js index 6a0bfdf8f49efe..0dff0eacb16068 100644 --- a/test/parallel/test-http2-large-write-destroy.js +++ b/test/parallel/test-http2-large-write-destroy.js @@ -16,6 +16,12 @@ const server = http2.createSecureServer({ cert: fixtures.readKey('agent1-cert.pem') }); server.on('stream', common.mustCall((stream) => { + stream.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + stream.respond({ 'Content-Type': 'application/octet-stream', 'Content-Length': (content.length.toString() * 2), diff --git a/test/parallel/test-http2-many-writes-and-destroy.js b/test/parallel/test-http2-many-writes-and-destroy.js index 78db76e001cd3a..273ea5e561481b 100644 --- a/test/parallel/test-http2-many-writes-and-destroy.js +++ b/test/parallel/test-http2-many-writes-and-destroy.js @@ -14,6 +14,11 @@ const http2 = require('http2'); const url = `http://localhost:${server.address().port}`; const client = http2.connect(url); const req = client.request({ ':method': 'POST' }); + req.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); for (let i = 0; i < 4000; i++) { req.write(Buffer.alloc(6)); diff --git a/test/parallel/test-http2-multistream-destroy-on-read-tls.js b/test/parallel/test-http2-multistream-destroy-on-read-tls.js index 91cbec6b2dc975..c86dd41e9e8fe1 100644 --- a/test/parallel/test-http2-multistream-destroy-on-read-tls.js +++ b/test/parallel/test-http2-multistream-destroy-on-read-tls.js @@ -17,6 +17,12 @@ const server = http2.createSecureServer({ const filenames = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']; server.on('stream', common.mustCall((stream) => { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); + function write() { stream.write('a'.repeat(10240)); stream.once('drain', write); @@ -35,6 +41,11 @@ server.listen(0, common.mustCall(() => { const stream = client.request({ ':path': `/${entry}` }); + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); stream.once('data', common.mustCall(() => { stream.destroy(); diff --git a/test/parallel/test-http2-options-max-reserved-streams.js b/test/parallel/test-http2-options-max-reserved-streams.js index 993bb1b7b3dfed..f9ae2e13281a46 100644 --- a/test/parallel/test-http2-options-max-reserved-streams.js +++ b/test/parallel/test-http2-options-max-reserved-streams.js @@ -34,8 +34,12 @@ server.on('stream', common.mustCall((stream) => { }, common.mustSucceed((pushedStream) => { pushedStream.respond(); pushedStream.on('aborted', common.mustCall()); - pushedStream.on('error', common.mustNotCall()); - pushedStream.on('close', common.mustCall(() => { + pushedStream.on('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); + pushedStream.on('close', common.mustNotCall(() => { assert.strictEqual(pushedStream.rstCode, 8); countdown.dec(); })); diff --git a/test/parallel/test-http2-respond-errors.js b/test/parallel/test-http2-respond-errors.js index 251e2a622e69b0..5c60e897c8d0b8 100644 --- a/test/parallel/test-http2-respond-errors.js +++ b/test/parallel/test-http2-respond-errors.js @@ -13,6 +13,11 @@ const server = http2.createServer(); Http2Stream.prototype.respond = () => 1; server.on('stream', common.mustCall((stream) => { + stream.on('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); // Send headers stream.respond({ 'content-type': 'text/plain' }); diff --git a/test/parallel/test-http2-respond-file-errors.js b/test/parallel/test-http2-respond-file-errors.js index fda59275e43e98..45bcf1021c65dc 100644 --- a/test/parallel/test-http2-respond-file-errors.js +++ b/test/parallel/test-http2-respond-file-errors.js @@ -29,6 +29,11 @@ const fname = fixtures.path('elipses.txt'); const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); // Check for all possible TypeError triggers on options Object.keys(optionsWithTypeError).forEach((option) => { diff --git a/test/parallel/test-http2-respond-file-fd-errors.js b/test/parallel/test-http2-respond-file-fd-errors.js index 67404c3350dd38..e64ad0042d681a 100644 --- a/test/parallel/test-http2-respond-file-fd-errors.js +++ b/test/parallel/test-http2-respond-file-fd-errors.js @@ -31,6 +31,12 @@ const fd = fs.openSync(fname, 'r'); const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + // Should throw if fd isn't a number Object.keys(types).forEach((type) => { if (type === 'number') { diff --git a/test/parallel/test-http2-server-rst-stream.js b/test/parallel/test-http2-server-rst-stream.js index 9f37ce71472353..09fbc91294a94d 100644 --- a/test/parallel/test-http2-server-rst-stream.js +++ b/test/parallel/test-http2-server-rst-stream.js @@ -19,7 +19,7 @@ const tests = [ [NGHTTP2_NO_ERROR, false], [NGHTTP2_NO_ERROR, false], [NGHTTP2_PROTOCOL_ERROR, true, 'NGHTTP2_PROTOCOL_ERROR'], - [NGHTTP2_CANCEL, false], + [NGHTTP2_CANCEL, true, 'NGHTTP2_CANCEL'], [NGHTTP2_REFUSED_STREAM, true, 'NGHTTP2_REFUSED_STREAM'], [NGHTTP2_INTERNAL_ERROR, true, 'NGHTTP2_INTERNAL_ERROR'], ]; diff --git a/test/parallel/test-http2-write-finishes-after-stream-destroy.js b/test/parallel/test-http2-write-finishes-after-stream-destroy.js index ed8833fdb926b1..f244315fd56b35 100644 --- a/test/parallel/test-http2-write-finishes-after-stream-destroy.js +++ b/test/parallel/test-http2-write-finishes-after-stream-destroy.js @@ -18,6 +18,8 @@ process.on('exit', global.gc); let serverSideHttp2StreamDestroyed = false; const server = http2.createServer(); server.on('stream', common.mustCall((stream, headers) => { + stream.once('error', common.mustCall()); + serverSideHttp2Stream = stream; stream.respond({ 'content-type': 'text/html', diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js index 71194f950a75af..c007c0ac41ec2c 100644 --- a/test/sequential/test-http2-max-session-memory.js +++ b/test/sequential/test-http2-max-session-memory.js @@ -13,7 +13,6 @@ const largeBuffer = Buffer.alloc(2e6); const server = http2.createServer({ maxSessionMemory: 1 }); server.on('stream', common.mustCall((stream) => { - stream.on('error', common.mustNotCall()); stream.respond(); stream.end(largeBuffer); })); @@ -28,9 +27,14 @@ server.listen(0, common.mustCall(() => { // This one should be rejected because the server is over budget // on the current memory allocation const req = client.request(); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM' + })); req.on('close', common.mustCall(() => { server.close(); - client.destroy(); + client.close(); })); }); From 3191fb762c7ef05d773e16cb9a5ca1e2e912c6a8 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Mon, 27 May 2024 13:06:07 +0200 Subject: [PATCH 10/52] fix two tests --- .../test-http2-options-max-reserved-streams.js | 2 +- .../parallel/test-http2-server-session-destroy.js | 2 +- test/parallel/test-http2-server-socket-destroy.js | 15 +++++++-------- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-http2-options-max-reserved-streams.js b/test/parallel/test-http2-options-max-reserved-streams.js index f9ae2e13281a46..71106d54a6b633 100644 --- a/test/parallel/test-http2-options-max-reserved-streams.js +++ b/test/parallel/test-http2-options-max-reserved-streams.js @@ -39,7 +39,7 @@ server.on('stream', common.mustCall((stream) => { code: 'ERR_HTTP2_STREAM_ERROR', message: 'Stream closed with error code NGHTTP2_CANCEL' })); - pushedStream.on('close', common.mustNotCall(() => { + pushedStream.on('close', common.mustCall(() => { assert.strictEqual(pushedStream.rstCode, 8); countdown.dec(); })); diff --git a/test/parallel/test-http2-server-session-destroy.js b/test/parallel/test-http2-server-session-destroy.js index 689edd00a6111c..06e2de0800aece 100644 --- a/test/parallel/test-http2-server-session-destroy.js +++ b/test/parallel/test-http2-server-session-destroy.js @@ -6,7 +6,7 @@ if (!common.hasCrypto) const h2 = require('http2'); const server = h2.createServer(); -server.once('stream', stream => { +server.once('stream', (stream) => { stream.once('error', common.expectsError({ name: 'Error', code: 'ERR_HTTP2_STREAM_ERROR', diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index c2addd8c9bdd8b..f47ad412777731 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -33,14 +33,13 @@ function onStream(stream) { assert.notStrictEqual(stream.session, undefined); - // With destroy(): - // The client may have an ECONNRESET or NGHTTP2_INTERNAL_ERROR error - // depending on the operating system, due mainly to differences - // in the timing of socket closing. The test would be flaky. - - // With resetAndDestroy(): - // TCP is always reset, no flakiness. - socket.resetAndDestroy(); + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); + + socket.destroy(); } server.listen(0); From b1905dd6f5e153ea0e26aff7f38100b8e3de346a Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Mon, 27 May 2024 23:59:39 +0200 Subject: [PATCH 11/52] fix goaway bugs --- lib/internal/http2/core.js | 30 +++++++++++-------- .../test-http2-server-shutdown-redundant.js | 6 ++++ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 54e013990dcb76..2a29bafa132556 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -678,14 +678,11 @@ function onGoawayData(code, lastStreamID, buf) { // If this is a no error goaway, begin shutting down. // No new streams permitted, but existing streams may // close naturally on their own. - session.close(); + session[kState].flags |= SESSION_FLAGS_CLOSED; + session[kMaybeDestroy](); } else { - // However, if the code is not NGHTTP_NO_ERROR, destroy the - // session immediately. We destroy with an error but send a - // goaway using NGHTTP2_NO_ERROR because there was no error - // condition on this side of the session that caused the - // shutdown. - session.destroy(new ERR_HTTP2_SESSION_ERROR(code), NGHTTP2_NO_ERROR); + // Must not send any further data. + closeSession(session, code, new ERR_HTTP2_SESSION_ERROR(code), true); } } @@ -848,6 +845,7 @@ function submitGoaway(code, lastStreamID, opaqueData) { if (this.destroyed) return; debugSessionObj(this, 'submitting goaway'); + this[kState].destroyCode = code; this[kUpdateTimer](); this[kHandle].goaway(code, lastStreamID, opaqueData); } @@ -1148,7 +1146,7 @@ function finishSessionClose(session, error) { } } -function closeSession(session, code, error) { +function closeSession(session, code, error, closed) { debugSessionObj(session, 'start closing/destroying', error); const state = session[kState]; @@ -1173,7 +1171,7 @@ function closeSession(session, code, error) { // Destroy the handle if it exists at this point. if (handle !== undefined) { handle.ondone = finishSessionClose.bind(null, session, error); - handle.destroy(code, socket.destroyed); + handle.destroy(code, closed ?? socket.destroyed); } else { finishSessionClose(session, error); } @@ -1511,6 +1509,9 @@ class Http2Session extends EventEmitter { // is only a notification, and does not affect the usable state of the // session with the notable exception that new incoming streams will // be rejected automatically. + // FIXME(szmarczak): goaway without connection close + // for error codes other than NO_ERROR is against the spec. + // See https://datatracker.ietf.org/doc/html/rfc9113#section-5.4.1 goaway(code = NGHTTP2_NO_ERROR, lastStreamID = 0, opaqueData) { if (this.destroyed) throw new ERR_HTTP2_INVALID_SESSION(); @@ -3296,9 +3297,14 @@ function socketOnClose() { debugSessionObj(session, 'socket closed'); const err = session.connecting ? new ERR_SOCKET_CLOSED() : null; const state = session[kState]; - const code = session[kType] === NGHTTP2_SESSION_SERVER ? - NGHTTP2_CANCEL : - NGHTTP2_INTERNAL_ERROR; + + let code = state.goawayCode ?? state.destroyCode; + if (code === NGHTTP2_NO_ERROR || code === null) { + code = session[kType] === NGHTTP2_SESSION_SERVER ? + NGHTTP2_CANCEL : + NGHTTP2_INTERNAL_ERROR; + } + state.streams.forEach((stream) => stream.close(code)); state.pendingStreams.forEach((stream) => stream.close(code)); session.close(); diff --git a/test/parallel/test-http2-server-shutdown-redundant.js b/test/parallel/test-http2-server-shutdown-redundant.js index 0f199bd68a1837..c8e7359c4396e1 100644 --- a/test/parallel/test-http2-server-shutdown-redundant.js +++ b/test/parallel/test-http2-server-shutdown-redundant.js @@ -9,6 +9,12 @@ const http2 = require('http2'); const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + const session = stream.session; session.goaway(1); session.goaway(2); From c2a96a7f191ee934cacfa1c4418d8b211320b857 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 28 May 2024 00:55:44 +0200 Subject: [PATCH 12/52] fix last test? ill check linux in a sec --- test/parallel/test-http2-client-destroy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index d6de649f1145b6..e40abe3e2818ab 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -138,7 +138,7 @@ const { getEventListeners } = require('events'); stream.on('error', common.expectsError({ code: 'ERR_HTTP2_STREAM_ERROR', name: 'Error', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + message: 'Stream closed with error code NGHTTP2_CANCEL' })); stream.once('aborted', common.mustCall()); client.destroy(); From 87c36ddf105f1a2a704a523ff1e47f73b067090a Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 28 May 2024 05:28:33 +0200 Subject: [PATCH 13/52] fix a bug --- lib/internal/http2/core.js | 10 ++++++++-- src/node_http2.cc | 14 ++++++++++++-- test/parallel/test-http2-server-session-destroy.js | 2 +- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 2a29bafa132556..c99d5cf4f44f33 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -682,7 +682,12 @@ function onGoawayData(code, lastStreamID, buf) { session[kMaybeDestroy](); } else { // Must not send any further data. - closeSession(session, code, new ERR_HTTP2_SESSION_ERROR(code), true); + closeSession( + session, + code, + new ERR_HTTP2_SESSION_ERROR(nameForErrorCode[code] || code), + true, + ); } } @@ -1542,7 +1547,8 @@ class Http2Session extends EventEmitter { code = error; error = code !== NGHTTP2_NO_ERROR ? - new ERR_HTTP2_SESSION_ERROR(code) : undefined; + new ERR_HTTP2_SESSION_ERROR(nameForErrorCode[code] || code) : + undefined; } if (code === undefined && error != null) code = NGHTTP2_INTERNAL_ERROR; diff --git a/src/node_http2.cc b/src/node_http2.cc index be36d2fa6930e7..f9d1000e877133 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -798,8 +798,18 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // the peer but the HTTP/2 spec recommends sending it anyway. We'll // make a best effort. if (!socket_closed) { - Debug(this, "terminating session with code %d", code); - CHECK_EQ(nghttp2_session_terminate_session(session_.get(), code), 0); + Debug(this, "goaway session with code %d", code); + int32_t lastStreamID = nghttp2_session_get_last_proc_stream_id( + session_.get()); + CHECK_EQ( + nghttp2_submit_goaway( + session_.get(), + NGHTTP2_FLAG_NONE, + lastStreamID, + code, + nullptr, + 0), + 0); SendPendingData(); } else if (stream_ != nullptr) { stream_->RemoveStreamListener(this); diff --git a/test/parallel/test-http2-server-session-destroy.js b/test/parallel/test-http2-server-session-destroy.js index 06e2de0800aece..c0467c8bead8a3 100644 --- a/test/parallel/test-http2-server-session-destroy.js +++ b/test/parallel/test-http2-server-session-destroy.js @@ -10,7 +10,7 @@ server.once('stream', (stream) => { stream.once('error', common.expectsError({ name: 'Error', code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + message: 'Stream closed with error code NGHTTP2_CANCEL' })); }); From 5eeb84933d65e0699c7411ff8aa20e50ab344c72 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 28 May 2024 05:54:13 +0200 Subject: [PATCH 14/52] format cpp --- src/node_http2.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index f9d1000e877133..4c77e921ed809c 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -799,16 +799,11 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // make a best effort. if (!socket_closed) { Debug(this, "goaway session with code %d", code); - int32_t lastStreamID = nghttp2_session_get_last_proc_stream_id( - session_.get()); + int32_t lastStreamID = + nghttp2_session_get_last_proc_stream_id(session_.get()); CHECK_EQ( nghttp2_submit_goaway( - session_.get(), - NGHTTP2_FLAG_NONE, - lastStreamID, - code, - nullptr, - 0), + session_.get(), NGHTTP2_FLAG_NONE, lastStreamID, code, nullptr, 0), 0); SendPendingData(); } else if (stream_ != nullptr) { From 22d9bb2dbeed50cf553a9ad763eb4eaf15ae84c5 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 10:24:49 +0200 Subject: [PATCH 15/52] fix tests --- benchmark/http2/headers.js | 2 +- .../test-http2-client-rststream-before-connect.js | 5 +++++ .../test-http2-options-max-headers-exceeds-nghttp2.js | 6 +++--- .../test-http2-propagate-session-destroy-code.js | 11 ++++++----- test/parallel/test-http2-server-sessionerror.js | 4 ++-- test/parallel/test-http2-too-many-settings.js | 2 +- 6 files changed, 18 insertions(+), 12 deletions(-) diff --git a/benchmark/http2/headers.js b/benchmark/http2/headers.js index ab890dbbf2573f..fe4f251e4d2ab9 100644 --- a/benchmark/http2/headers.js +++ b/benchmark/http2/headers.js @@ -45,7 +45,7 @@ function main({ n, nheaders }) { } else { bench.end(n); server.close(); - client.destroy(); + client.close(); } }); } diff --git a/test/parallel/test-http2-client-rststream-before-connect.js b/test/parallel/test-http2-client-rststream-before-connect.js index bc0cb5ff619dc0..1ff56cf71ff440 100644 --- a/test/parallel/test-http2-client-rststream-before-connect.js +++ b/test/parallel/test-http2-client-rststream-before-connect.js @@ -8,6 +8,11 @@ const h2 = require('http2'); const server = h2.createServer(); server.on('stream', (stream) => { + stream.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_PROTOCOL_ERROR' + })); stream.on('close', common.mustCall()); stream.respond(); stream.end('ok'); diff --git a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js index df3aefff11e7ad..3d651819af9cf0 100644 --- a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js +++ b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js @@ -25,7 +25,7 @@ server.listen(0, common.mustCall(() => { client.on('error', common.expectsError({ code: 'ERR_HTTP2_SESSION_ERROR', name: 'Error', - message: 'Session closed with error code 9' + message: 'Session closed with error code NGHTTP2_COMPRESSION_ERROR' })); const req = client.request({ @@ -42,7 +42,7 @@ server.listen(0, common.mustCall(() => { req.on('error', common.expectsError({ code: 'ERR_HTTP2_SESSION_ERROR', name: 'Error', - message: 'Session closed with error code 9' + message: 'Session closed with error code NGHTTP2_COMPRESSION_ERROR' })); req.end(); })); @@ -78,7 +78,7 @@ server.listen(0, common.mustCall(() => { common.mustCall((err, session) => { assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); assert.strictEqual(err.name, 'Error'); - assert.strictEqual(err.message, 'Session closed with error code 9'); + assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_COMPRESSION_ERROR'); assert.strictEqual(session instanceof ServerHttp2Session, true); server.close(); }), diff --git a/test/parallel/test-http2-propagate-session-destroy-code.js b/test/parallel/test-http2-propagate-session-destroy-code.js index c54e52e085af4b..3e61738d9e7b9f 100644 --- a/test/parallel/test-http2-propagate-session-destroy-code.js +++ b/test/parallel/test-http2-propagate-session-destroy-code.js @@ -7,7 +7,8 @@ if (!common.hasCrypto) const assert = require('assert'); const http2 = require('http2'); const server = http2.createServer(); -const errRegEx = /Session closed with error code 7/; +const sessionErrorMessage = 'Session closed with error code NGHTTP2_REFUSED_STREAM'; +const streamErrorMessage = 'Stream closed with error code NGHTTP2_REFUSED_STREAM'; const destroyCode = http2.constants.NGHTTP2_REFUSED_STREAM; server.on('error', common.mustNotCall()); @@ -15,14 +16,14 @@ server.on('error', common.mustNotCall()); server.on('session', (session) => { session.on('close', common.mustCall()); session.on('error', common.mustCall((err) => { - assert.match(err.message, errRegEx); + assert.strictEqual(err.message, sessionErrorMessage); assert.strictEqual(session.closed, false); assert.strictEqual(session.destroyed, true); })); session.on('stream', common.mustCall((stream) => { stream.on('error', common.mustCall((err) => { - assert.match(err.message, errRegEx); + assert.strict(err.message, streamErrorMessage); assert.strictEqual(session.closed, false); assert.strictEqual(session.destroyed, true); assert.strictEqual(stream.rstCode, destroyCode); @@ -36,7 +37,7 @@ server.listen(0, common.mustCall(() => { const session = http2.connect(`http://localhost:${server.address().port}`); session.on('error', common.mustCall((err) => { - assert.match(err.message, errRegEx); + assert.strictEqual(err.message, sessionErrorMessage); assert.strictEqual(session.closed, false); assert.strictEqual(session.destroyed, true); })); @@ -44,7 +45,7 @@ server.listen(0, common.mustCall(() => { const stream = session.request({ [http2.constants.HTTP2_HEADER_PATH]: '/' }); stream.on('error', common.mustCall((err) => { - assert.match(err.message, errRegEx); + assert.strictEqual(err.message, streamErrorMessage); assert.strictEqual(stream.rstCode, destroyCode); })); diff --git a/test/parallel/test-http2-server-sessionerror.js b/test/parallel/test-http2-server-sessionerror.js index b71b9df80f083f..0800a6d7c35480 100644 --- a/test/parallel/test-http2-server-sessionerror.js +++ b/test/parallel/test-http2-server-sessionerror.js @@ -47,7 +47,7 @@ server.listen(0, common.mustCall(() => { .on('error', common.mustCall((err) => { if (err.code !== 'ECONNRESET') { assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); - assert.strictEqual(err.message, 'Session closed with error code 2'); + assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); } })) .on('close', () => { @@ -56,7 +56,7 @@ server.listen(0, common.mustCall(() => { .on('error', common.mustCall((err) => { if (err.code !== 'ECONNRESET') { assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); - assert.strictEqual(err.message, 'Session closed with error code 2'); + assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); } })) .on('close', () => server.close()); diff --git a/test/parallel/test-http2-too-many-settings.js b/test/parallel/test-http2-too-many-settings.js index da5d5865792a83..4a5697f3e63193 100644 --- a/test/parallel/test-http2-too-many-settings.js +++ b/test/parallel/test-http2-too-many-settings.js @@ -32,7 +32,7 @@ function doTest(session) { client.on('error', common.mustCall((err) => { if (err.code !== 'ECONNRESET') { assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); - assert.strictEqual(err.message, 'Session closed with error code 2'); + assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); } })); client.on('close', common.mustCall(() => server.close())); From a9b456e3cadad10927dc08f14392fdfe7fe79a03 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 11:23:34 +0200 Subject: [PATCH 16/52] NGHTTP2_CONNECT_ERROR --- lib/internal/http2/core.js | 7 +++++++ test/parallel/test-http2-client-connection-tunnelling.js | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index c99d5cf4f44f33..77a278beca52cb 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -264,6 +264,7 @@ const { const { NGHTTP2_CANCEL, + NGHTTP2_CONNECT_ERROR, NGHTTP2_REFUSED_STREAM, NGHTTP2_DEFAULT_WEIGHT, NGHTTP2_FLAG_END_STREAM, @@ -2333,6 +2334,8 @@ class Http2Stream extends Duplex { destroyCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; } + const connect = this[kSentHeaders]?.[HTTP2_HEADER_METHOD] === HTTP2_METHOD_CONNECT; + const sessionCode = goawayCode ?? destroyCode; // If a stream has already closed successfully, there is no error @@ -2356,6 +2359,10 @@ class Http2Stream extends Duplex { if (!this.closed) { const rstStreamStatus = hasHandle ? kForceRstStream : kNoRstStream; + if (code === null && connect) { + code = NGHTTP2_CONNECT_ERROR; + } + const defaultCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; code ??= defaultCode; diff --git a/test/parallel/test-http2-client-connection-tunnelling.js b/test/parallel/test-http2-client-connection-tunnelling.js index cb2887efffc05f..296f5615421f05 100644 --- a/test/parallel/test-http2-client-connection-tunnelling.js +++ b/test/parallel/test-http2-client-connection-tunnelling.js @@ -66,7 +66,7 @@ netServer.listen(0, common.mustCall(() => { tlsSocket.once('error', common.expectsError({ name: 'Error', code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' + message: 'Stream closed with error code NGHTTP2_CONNECT_ERROR' })); // Forcibly kill the TLS socket From 0c32c9282f09123fe719ed68181edb55849bd84c Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 11:50:02 +0200 Subject: [PATCH 17/52] test econnreset --- ...st-http2-client-connection-tunnel-reset.js | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 test/parallel/test-http2-client-connection-tunnel-reset.js diff --git a/test/parallel/test-http2-client-connection-tunnel-reset.js b/test/parallel/test-http2-client-connection-tunnel-reset.js new file mode 100644 index 00000000000000..676582c82a293c --- /dev/null +++ b/test/parallel/test-http2-client-connection-tunnel-reset.js @@ -0,0 +1,36 @@ +// Flags: --expose-internals + +'use strict'; + +const common = require('../common'); +const h2 = require('http2'); + +const { kSocket } = require('internal/http2/util'); + +const h2Server = h2.createServer(() => {}); + +h2Server.on('connect', (req, res) => { + res.writeHead(200, {}); + + res.stream.session[kSocket].resetAndDestroy(); + + h2Server.close(); +}); + +h2Server.listen(0, common.mustCall(() => { + const proxyClient = h2.connect(`http://localhost:${h2Server.address().port}`); + proxyClient.once('error', common.expectsError({ + name: 'Error', + code: 'ECONNRESET', + message: 'read ECONNRESET' + })); + const proxyReq = proxyClient.request({ + ':method': 'CONNECT', + ':authority': 'example.com:443' + }); + proxyReq.once('error', common.expectsError({ + name: 'Error', + code: 'ECONNRESET', + message: 'read ECONNRESET' + })); +})); From e67926d3461dfc4a7c877667f34387b04cad12a5 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 12:11:22 +0200 Subject: [PATCH 18/52] [skip ci] hascrypto --- test/parallel/test-http2-client-connection-tunnel-reset.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-http2-client-connection-tunnel-reset.js b/test/parallel/test-http2-client-connection-tunnel-reset.js index 676582c82a293c..83614525ae6256 100644 --- a/test/parallel/test-http2-client-connection-tunnel-reset.js +++ b/test/parallel/test-http2-client-connection-tunnel-reset.js @@ -3,8 +3,9 @@ 'use strict'; const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); const h2 = require('http2'); - const { kSocket } = require('internal/http2/util'); const h2Server = h2.createServer(() => {}); From 2ed1755fbb09918c69056f34849324b15f8045ef Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 15:42:37 +0200 Subject: [PATCH 19/52] bug fixes --- lib/internal/http2/core.js | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 77a278beca52cb..5cba2901905a56 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -550,7 +550,8 @@ function onStreamClose(code) { // Defer destroy we actually emit end. if (!stream.readable || code !== NGHTTP2_NO_ERROR) { // If errored or ended, we can destroy immediately. - stream.destroy(); + const error = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); + stream.destroy(error); } else { // Wait for end to destroy. stream.on('end', stream[kMaybeDestroy]); @@ -682,6 +683,8 @@ function onGoawayData(code, lastStreamID, buf) { session[kState].flags |= SESSION_FLAGS_CLOSED; session[kMaybeDestroy](); } else { + session[kState].flags |= SESSION_FLAGS_DESTROYED; + // Must not send any further data. closeSession( session, @@ -1574,11 +1577,16 @@ class Http2Session extends EventEmitter { if (this.closed || this.destroyed) return; debugSessionObj(this, 'marking session closed'); - this[kState].flags |= SESSION_FLAGS_CLOSED; + const state = this[kState]; + state.flags |= SESSION_FLAGS_CLOSED; if (typeof callback === 'function') this.once('close', callback); - this.goaway(); - this[kMaybeDestroy](); + + if (state.streams.size > 0 || state.pendingStreams.size > 0) { + this.goaway(); + } else { + this.destroy(); + } } [EventEmitter.captureRejectionSymbol](err, event, ...args) { @@ -2382,10 +2390,6 @@ class Http2Stream extends Duplex { sessionState.writeQueueSize -= state.writeQueueSize; state.writeQueueSize = 0; - if (err == null && code !== NGHTTP2_NO_ERROR) { - err = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); - } - this[kSession] = undefined; this[kHandle] = undefined; From 3334e47cbf1d340991d0e04bbb8bf53a47297708 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 15:50:45 +0200 Subject: [PATCH 20/52] fix --- lib/internal/http2/core.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 5cba2901905a56..8a24c2106caced 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -549,8 +549,12 @@ function onStreamClose(code) { stream[kState].fd = -1; // Defer destroy we actually emit end. if (!stream.readable || code !== NGHTTP2_NO_ERROR) { + let error; + if (code !== NGHTTP2_NO_ERROR) { + error = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); + } + // If errored or ended, we can destroy immediately. - const error = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); stream.destroy(error); } else { // Wait for end to destroy. From 14f637187e11b0d486de8ee87f4e6f393679e50e Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 16:37:29 +0200 Subject: [PATCH 21/52] fix errors --- ...test-http2-client-connection-tunnelling.js | 6 ----- test/parallel/test-http2-client-destroy.js | 22 +------------------ ...t-http2-client-rststream-before-connect.js | 6 ----- .../test-http2-client-socket-destroy.js | 5 ----- .../test-http2-close-while-writing.js | 5 ----- test/parallel/test-http2-compat-aborted.js | 5 ----- test/parallel/test-http2-compat-errors.js | 5 ----- .../test-http2-compat-serverresponse-close.js | 5 ----- .../test-http2-destroy-after-write.js | 5 ----- .../test-http2-large-write-destroy.js | 6 ----- .../test-http2-many-writes-and-destroy.js | 5 ----- ...t-http2-multistream-destroy-on-read-tls.js | 5 ----- test/parallel/test-http2-respond-errors.js | 6 ----- .../test-http2-respond-file-errors.js | 6 ----- .../test-http2-respond-file-fd-errors.js | 6 ----- test/parallel/test-http2-server-errors.js | 10 --------- .../test-http2-server-session-destroy.js | 4 ---- ...st-http2-server-shutdown-options-errors.js | 6 ----- ...est-http2-server-stream-session-destroy.js | 9 ++------ 19 files changed, 3 insertions(+), 124 deletions(-) diff --git a/test/parallel/test-http2-client-connection-tunnelling.js b/test/parallel/test-http2-client-connection-tunnelling.js index 296f5615421f05..6e04121ca71ea8 100644 --- a/test/parallel/test-http2-client-connection-tunnelling.js +++ b/test/parallel/test-http2-client-connection-tunnelling.js @@ -63,12 +63,6 @@ netServer.listen(0, common.mustCall(() => { netServer.close(); })); - tlsSocket.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CONNECT_ERROR' - })); - // Forcibly kill the TLS socket tlsSocket.destroy(); diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index e40abe3e2818ab..89d123d0f6df7c 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -106,11 +106,6 @@ const { getEventListeners } = require('events'); { const server = h2.createServer(); server.on('stream', common.mustCall((stream) => { - stream.once('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); stream.session.destroy(); })); @@ -151,12 +146,7 @@ const { getEventListeners } = require('events'); server.close(); }); - const req = client.request(); - req.once('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); + client.request(); })); } @@ -174,11 +164,6 @@ const { getEventListeners } = require('events'); })); const req = client.request(); - req.once('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); req.destroy(); })); } @@ -197,11 +182,6 @@ const { getEventListeners } = require('events'); })); const req = client.request({}, { endStream: false }); - req.once('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); req.destroy(); })); } diff --git a/test/parallel/test-http2-client-rststream-before-connect.js b/test/parallel/test-http2-client-rststream-before-connect.js index 1ff56cf71ff440..ac9e6176d2bfb4 100644 --- a/test/parallel/test-http2-client-rststream-before-connect.js +++ b/test/parallel/test-http2-client-rststream-before-connect.js @@ -61,12 +61,6 @@ server.listen(0, common.mustCall(() => { client.close(); })); - req.on('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_PROTOCOL_ERROR' - })); - // The `response` event should not fire as the server should receive the // RST_STREAM frame before it ever has a chance to reply. req.on('response', common.mustNotCall()); diff --git a/test/parallel/test-http2-client-socket-destroy.js b/test/parallel/test-http2-client-socket-destroy.js index 795d98e81212f3..52cccad787e563 100644 --- a/test/parallel/test-http2-client-socket-destroy.js +++ b/test/parallel/test-http2-client-socket-destroy.js @@ -16,11 +16,6 @@ const server = h2.createServer(); // We use the lower-level API here server.on('stream', common.mustCall((stream) => { stream.on('aborted', common.mustCall()); - stream.on('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); stream.on('close', common.mustCall()); stream.respond(); stream.write(body); diff --git a/test/parallel/test-http2-close-while-writing.js b/test/parallel/test-http2-close-while-writing.js index 1eddfbaf0af074..42a4b365e4f88b 100644 --- a/test/parallel/test-http2-close-while-writing.js +++ b/test/parallel/test-http2-close-while-writing.js @@ -42,11 +42,6 @@ server.listen(0, function() { maxSessionMemory: 1000 }); client_stream = client.request({ ':method': 'POST' }); - client_stream.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); client_stream.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-compat-aborted.js b/test/parallel/test-http2-compat-aborted.js index 3df1f57baf998d..5c525b4d3171db 100644 --- a/test/parallel/test-http2-compat-aborted.js +++ b/test/parallel/test-http2-compat-aborted.js @@ -22,11 +22,6 @@ server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { const request = client.request({}, { endStream: false }); - request.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-errors.js b/test/parallel/test-http2-compat-errors.js index 2d357002366f95..2dbea121c962b1 100644 --- a/test/parallel/test-http2-compat-errors.js +++ b/test/parallel/test-http2-compat-errors.js @@ -28,11 +28,6 @@ server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { const request = client.request({}, { endStream: false }); - request.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-close.js b/test/parallel/test-http2-compat-serverresponse-close.js index 5ba40e5ddf09cc..18922bee4d94a9 100644 --- a/test/parallel/test-http2-compat-serverresponse-close.js +++ b/test/parallel/test-http2-compat-serverresponse-close.js @@ -23,11 +23,6 @@ server.on('listening', () => { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { const request = client.request({}, { endStream: false }); - request.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); request.on('data', common.mustCall(function(chunk) { client.destroy(); server.close(); diff --git a/test/parallel/test-http2-destroy-after-write.js b/test/parallel/test-http2-destroy-after-write.js index d3324ac7ff340d..d1b6b57da86e41 100644 --- a/test/parallel/test-http2-destroy-after-write.js +++ b/test/parallel/test-http2-destroy-after-write.js @@ -11,11 +11,6 @@ const server = http2.createServer(); server.on('session', common.mustCall(function(session) { session.on('stream', common.mustCall(function(stream) { - stream.once('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); stream.on('end', common.mustCall(function() { this.respond({ ':status': 200 diff --git a/test/parallel/test-http2-large-write-destroy.js b/test/parallel/test-http2-large-write-destroy.js index 0dff0eacb16068..6a0bfdf8f49efe 100644 --- a/test/parallel/test-http2-large-write-destroy.js +++ b/test/parallel/test-http2-large-write-destroy.js @@ -16,12 +16,6 @@ const server = http2.createSecureServer({ cert: fixtures.readKey('agent1-cert.pem') }); server.on('stream', common.mustCall((stream) => { - stream.once('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); - stream.respond({ 'Content-Type': 'application/octet-stream', 'Content-Length': (content.length.toString() * 2), diff --git a/test/parallel/test-http2-many-writes-and-destroy.js b/test/parallel/test-http2-many-writes-and-destroy.js index 273ea5e561481b..78db76e001cd3a 100644 --- a/test/parallel/test-http2-many-writes-and-destroy.js +++ b/test/parallel/test-http2-many-writes-and-destroy.js @@ -14,11 +14,6 @@ const http2 = require('http2'); const url = `http://localhost:${server.address().port}`; const client = http2.connect(url); const req = client.request({ ':method': 'POST' }); - req.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); for (let i = 0; i < 4000; i++) { req.write(Buffer.alloc(6)); diff --git a/test/parallel/test-http2-multistream-destroy-on-read-tls.js b/test/parallel/test-http2-multistream-destroy-on-read-tls.js index c86dd41e9e8fe1..c7ce1b19c96dab 100644 --- a/test/parallel/test-http2-multistream-destroy-on-read-tls.js +++ b/test/parallel/test-http2-multistream-destroy-on-read-tls.js @@ -41,11 +41,6 @@ server.listen(0, common.mustCall(() => { const stream = client.request({ ':path': `/${entry}` }); - stream.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); stream.once('data', common.mustCall(() => { stream.destroy(); diff --git a/test/parallel/test-http2-respond-errors.js b/test/parallel/test-http2-respond-errors.js index 5c60e897c8d0b8..431ee34f1ad7bc 100644 --- a/test/parallel/test-http2-respond-errors.js +++ b/test/parallel/test-http2-respond-errors.js @@ -13,12 +13,6 @@ const server = http2.createServer(); Http2Stream.prototype.respond = () => 1; server.on('stream', common.mustCall((stream) => { - stream.on('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); - // Send headers stream.respond({ 'content-type': 'text/plain' }); diff --git a/test/parallel/test-http2-respond-file-errors.js b/test/parallel/test-http2-respond-file-errors.js index 45bcf1021c65dc..d755f781ab6f9b 100644 --- a/test/parallel/test-http2-respond-file-errors.js +++ b/test/parallel/test-http2-respond-file-errors.js @@ -29,12 +29,6 @@ const fname = fixtures.path('elipses.txt'); const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { - stream.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); - // Check for all possible TypeError triggers on options Object.keys(optionsWithTypeError).forEach((option) => { Object.keys(types).forEach((type) => { diff --git a/test/parallel/test-http2-respond-file-fd-errors.js b/test/parallel/test-http2-respond-file-fd-errors.js index e64ad0042d681a..67404c3350dd38 100644 --- a/test/parallel/test-http2-respond-file-fd-errors.js +++ b/test/parallel/test-http2-respond-file-fd-errors.js @@ -31,12 +31,6 @@ const fd = fs.openSync(fname, 'r'); const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { - stream.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); - // Should throw if fd isn't a number Object.keys(types).forEach((type) => { if (type === 'number') { diff --git a/test/parallel/test-http2-server-errors.js b/test/parallel/test-http2-server-errors.js index f9a88307306310..959ddccdc7c28a 100644 --- a/test/parallel/test-http2-server-errors.js +++ b/test/parallel/test-http2-server-errors.js @@ -38,11 +38,6 @@ const h2 = require('http2'); ':authority': `localhost:${port}`, }; const request = client.request(headers); - request.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); request.on('data', common.mustCall(function(chunk) { // Cause an error on the server side client.destroy(); @@ -83,11 +78,6 @@ const h2 = require('http2'); ':authority': `localhost:${port}`, }; const request = client.request(headers); - request.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' - })); request.on('data', common.mustCall(function(chunk) { // Cause an error on the server side client.destroy(); diff --git a/test/parallel/test-http2-server-session-destroy.js b/test/parallel/test-http2-server-session-destroy.js index c0467c8bead8a3..10ef88856a387a 100644 --- a/test/parallel/test-http2-server-session-destroy.js +++ b/test/parallel/test-http2-server-session-destroy.js @@ -19,10 +19,6 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => { session.request({ ':method': 'POST' }).end(common.mustCall(() => { session.destroy(); server.close(); - })).once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' })); }); diff --git a/test/parallel/test-http2-server-shutdown-options-errors.js b/test/parallel/test-http2-server-shutdown-options-errors.js index 654a0b0ddc6b49..e1c426e2823e75 100644 --- a/test/parallel/test-http2-server-shutdown-options-errors.js +++ b/test/parallel/test-http2-server-shutdown-options-errors.js @@ -17,12 +17,6 @@ const types = [ ]; server.on('stream', common.mustCall((stream) => { - stream.once('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); - const session = stream.session; for (const input of types) { diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js index 14d12a33f686e0..5a7053b37f1d97 100644 --- a/test/parallel/test-http2-server-stream-session-destroy.js +++ b/test/parallel/test-http2-server-stream-session-destroy.js @@ -34,13 +34,8 @@ server.on('stream', common.mustCall((stream) => { name: 'Error' } ); - // When session is forcibly destroyed, - // all streams are destroyed with an error. - stream.on('error', common.expectsError({ - name: 'Error', - code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); + // When session is destroyed all streams are destroyed and no further + // error should be emitted. assert.strictEqual(stream.write('data', common.expectsError({ name: 'Error', code: 'ERR_STREAM_WRITE_AFTER_END', From de563b15bec7a1696a1e70800b9da8e86c44b474 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 16:53:25 +0200 Subject: [PATCH 22/52] more verbose error checks --- test/parallel/test-http2-respond-errors.js | 6 +++++- test/parallel/test-http2-respond-file-errors.js | 6 +++++- test/parallel/test-http2-respond-file-fd-errors.js | 6 +++++- test/parallel/test-http2-server-shutdown-options-errors.js | 6 +++++- .../test-http2-write-finishes-after-stream-destroy.js | 2 -- 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-http2-respond-errors.js b/test/parallel/test-http2-respond-errors.js index 431ee34f1ad7bc..b90b2d7b89f4d8 100644 --- a/test/parallel/test-http2-respond-errors.js +++ b/test/parallel/test-http2-respond-errors.js @@ -42,7 +42,11 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); - req.once('error', common.mustCall()); + req.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.on('end', common.mustNotCall()); req.on('close', common.mustCall(() => { client.close(); diff --git a/test/parallel/test-http2-respond-file-errors.js b/test/parallel/test-http2-respond-file-errors.js index d755f781ab6f9b..c63017f8335ad9 100644 --- a/test/parallel/test-http2-respond-file-errors.js +++ b/test/parallel/test-http2-respond-file-errors.js @@ -93,7 +93,11 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); - req.once('error', common.mustCall()); + req.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-respond-file-fd-errors.js b/test/parallel/test-http2-respond-file-fd-errors.js index 67404c3350dd38..43395b9bd985eb 100644 --- a/test/parallel/test-http2-respond-file-fd-errors.js +++ b/test/parallel/test-http2-respond-file-fd-errors.js @@ -117,7 +117,11 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); - req.once('error', common.mustCall()); + req.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-server-shutdown-options-errors.js b/test/parallel/test-http2-server-shutdown-options-errors.js index e1c426e2823e75..c3f53e55e05d07 100644 --- a/test/parallel/test-http2-server-shutdown-options-errors.js +++ b/test/parallel/test-http2-server-shutdown-options-errors.js @@ -59,7 +59,11 @@ server.listen( const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); req.resume(); - req.once('error', common.mustCall()); + req.once('error', common.expectsError({ + name: 'Error', + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); req.on('close', common.mustCall(() => { client.close(); server.close(); diff --git a/test/parallel/test-http2-write-finishes-after-stream-destroy.js b/test/parallel/test-http2-write-finishes-after-stream-destroy.js index f244315fd56b35..ed8833fdb926b1 100644 --- a/test/parallel/test-http2-write-finishes-after-stream-destroy.js +++ b/test/parallel/test-http2-write-finishes-after-stream-destroy.js @@ -18,8 +18,6 @@ process.on('exit', global.gc); let serverSideHttp2StreamDestroyed = false; const server = http2.createServer(); server.on('stream', common.mustCall((stream, headers) => { - stream.once('error', common.mustCall()); - serverSideHttp2Stream = stream; stream.respond({ 'content-type': 'text/html', From 107dc65fdeb13798e39684a0cce117f4b6cd6435 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 17:32:26 +0200 Subject: [PATCH 23/52] refactor _destroy --- lib/internal/http2/core.js | 63 +++++++------------ test/parallel/test-http2-compat-aborted.js | 2 +- test/parallel/test-http2-compat-errors.js | 2 +- .../test-http2-compat-serverresponse-close.js | 2 +- 4 files changed, 27 insertions(+), 42 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 8a24c2106caced..8b282cb41b29e9 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2324,53 +2324,37 @@ class Http2Stream extends Duplex { // This will cause the internal resources to be released. // * Then cleans up the resources on the js side _destroy(err, callback) { + const id = this[kID]; const session = this[kSession]; const handle = this[kHandle]; - const id = this[kID]; - - debugStream(this[kID] || 'pending', session[kType], 'destroying stream'); - - const state = this[kState]; + const hasHandle = handle !== undefined; const sessionState = session[kState]; - const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; - let goawayCode = sessionState.goawayCode; - if (goawayCode === NGHTTP2_NO_ERROR) { - // Received NO_ERROR (GOAWAY) from remote peer yet still got reset - goawayCode = NGHTTP2_INTERNAL_ERROR; - } + debugStream(id || 'pending', session[kType], 'destroying stream'); - let destroyCode = sessionState.destroyCode; - if (destroyCode === NGHTTP2_NO_ERROR) { - // Code to send to remote peer - destroyCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; - } - - const connect = this[kSentHeaders]?.[HTTP2_HEADER_METHOD] === HTTP2_METHOD_CONNECT; + if (!this.closed) { + const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; - const sessionCode = goawayCode ?? destroyCode; + let goawayCode = sessionState.goawayCode; + if (goawayCode === NGHTTP2_NO_ERROR) { + // Received NO_ERROR (GOAWAY) from remote peer yet still got reset + goawayCode = NGHTTP2_INTERNAL_ERROR; + } - // If a stream has already closed successfully, there is no error - // to report from this stream, even if the session has errored. - // This can happen if the stream was already in process of destroying - // after a successful close, but the session had a error between - // this stream's close and destroy operations. - // Previously, this always overrode a successful close operation code - // NGHTTP2_NO_ERROR (0) with sessionCode because the use of the || operator. - let code = this.rstCode ?? sessionCode; - if (err != null && code === null) { - if (err instanceof AbortError) { - // Enables using AbortController to cancel requests with RST code 8. - code = NGHTTP2_CANCEL; - } else { - code = NGHTTP2_INTERNAL_ERROR; + let destroyCode = sessionState.destroyCode; + if (destroyCode === NGHTTP2_NO_ERROR) { + // Code to send to remote peer + destroyCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; } - } - const hasHandle = handle !== undefined; - if (!this.closed) { - const rstStreamStatus = hasHandle ? kForceRstStream : kNoRstStream; + const sessionCode = goawayCode ?? destroyCode; + let code = this.rstCode ?? sessionCode; + + if (err != null && code === null) { + code = err instanceof AbortError ? NGHTTP2_CANCEL : NGHTTP2_INTERNAL_ERROR; + } + const connect = this[kSentHeaders]?.[HTTP2_HEADER_METHOD] === HTTP2_METHOD_CONNECT; if (code === null && connect) { code = NGHTTP2_CONNECT_ERROR; } @@ -2378,6 +2362,7 @@ class Http2Stream extends Duplex { const defaultCode = isServerSession ? NGHTTP2_INTERNAL_ERROR : NGHTTP2_CANCEL; code ??= defaultCode; + const rstStreamStatus = hasHandle ? kForceRstStream : kNoRstStream; closeStream(this, code, rstStreamStatus); } @@ -2391,8 +2376,8 @@ class Http2Stream extends Duplex { } // Adjust the write queue size for accounting - sessionState.writeQueueSize -= state.writeQueueSize; - state.writeQueueSize = 0; + sessionState.writeQueueSize -= this[kState].writeQueueSize; + this[kState].writeQueueSize = 0; this[kSession] = undefined; this[kHandle] = undefined; diff --git a/test/parallel/test-http2-compat-aborted.js b/test/parallel/test-http2-compat-aborted.js index 5c525b4d3171db..c7e9b89863327b 100644 --- a/test/parallel/test-http2-compat-aborted.js +++ b/test/parallel/test-http2-compat-aborted.js @@ -21,7 +21,7 @@ const server = h2.createServer(common.mustCall(function(req, res) { server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request({}, { endStream: false }); + const request = client.request({}); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-errors.js b/test/parallel/test-http2-compat-errors.js index 2dbea121c962b1..b3628d245373ff 100644 --- a/test/parallel/test-http2-compat-errors.js +++ b/test/parallel/test-http2-compat-errors.js @@ -27,7 +27,7 @@ const server = h2.createServer(common.mustCall(function(req, res) { server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request({}, { endStream: false }); + const request = client.request({}); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-close.js b/test/parallel/test-http2-compat-serverresponse-close.js index 18922bee4d94a9..11e1ee24463d70 100644 --- a/test/parallel/test-http2-compat-serverresponse-close.js +++ b/test/parallel/test-http2-compat-serverresponse-close.js @@ -22,7 +22,7 @@ server.listen(0); server.on('listening', () => { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request({}, { endStream: false }); + const request = client.request({}); request.on('data', common.mustCall(function(chunk) { client.destroy(); server.close(); From d44fbbb4cc3f675936a8d4d357da2173f23e2833 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 29 May 2024 17:34:08 +0200 Subject: [PATCH 24/52] less diff --- test/parallel/test-http2-compat-aborted.js | 2 +- test/parallel/test-http2-compat-errors.js | 2 +- test/parallel/test-http2-compat-serverresponse-close.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-http2-compat-aborted.js b/test/parallel/test-http2-compat-aborted.js index c7e9b89863327b..0ed0d800435f38 100644 --- a/test/parallel/test-http2-compat-aborted.js +++ b/test/parallel/test-http2-compat-aborted.js @@ -21,7 +21,7 @@ const server = h2.createServer(common.mustCall(function(req, res) { server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request({}); + const request = client.request(); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-errors.js b/test/parallel/test-http2-compat-errors.js index b3628d245373ff..18dc385422a48e 100644 --- a/test/parallel/test-http2-compat-errors.js +++ b/test/parallel/test-http2-compat-errors.js @@ -27,7 +27,7 @@ const server = h2.createServer(common.mustCall(function(req, res) { server.listen(0, common.mustCall(function() { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request({}); + const request = client.request(); request.on('data', common.mustCall((chunk) => { client.destroy(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-close.js b/test/parallel/test-http2-compat-serverresponse-close.js index 11e1ee24463d70..71079f425c97fc 100644 --- a/test/parallel/test-http2-compat-serverresponse-close.js +++ b/test/parallel/test-http2-compat-serverresponse-close.js @@ -22,7 +22,7 @@ server.listen(0); server.on('listening', () => { const url = `http://localhost:${server.address().port}`; const client = h2.connect(url, common.mustCall(() => { - const request = client.request({}); + const request = client.request(); request.on('data', common.mustCall(function(chunk) { client.destroy(); server.close(); From 89946994cd799e9881e6a6b2d07f169f0211a759 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 14:50:30 +0200 Subject: [PATCH 25/52] fix dropped rsts and goaways --- src/node_http2.cc | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 4c77e921ed809c..411e2adaf2b6b3 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1725,6 +1725,18 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { stream_->ReadStart(); } + // If there is more incoming data queued up, consume it. + if (stream_buf_offset_ > 0) { + ConsumeHTTP2Data(); + } + + // This needs to be before is_destroyed(), + // so queued GOAWAY gets sent. + if (!is_write_scheduled()) { + // Schedule a new write if nghttp2 wants to send data. + MaybeScheduleWrite(); + } + if (is_destroyed()) { HandleScope scope(env()->isolate()); MakeCallback(env()->ondone_string(), 0, nullptr); @@ -1735,16 +1747,6 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { } return; } - - // If there is more incoming data queued up, consume it. - if (stream_buf_offset_ > 0) { - ConsumeHTTP2Data(); - } - - if (!is_write_scheduled() && !is_destroyed()) { - // Schedule a new write if nghttp2 wants to send data. - MaybeScheduleWrite(); - } } // If the underlying nghttp2_session struct has data pending in its outbound @@ -1861,11 +1863,10 @@ void Http2Session::CopyDataIntoOutgoing(const uint8_t* src, size_t src_length) { // Returns non-zero value if a write is already in progress. uint8_t Http2Session::SendPendingData() { Debug(this, "sending pending data"); - // Do not attempt to send data on the socket if the destroying flag has - // been set. That means everything is shutting down and the socket - // will not be usable. - if (is_destroyed()) - return 0; + + // Must not early return if is_destroyed(), + // otherwise queued GOAWAY gets dropped! + set_write_scheduled(false); // SendPendingData should not be called recursively. From a48b01f187511146bb8b8f53a37b66063dd1dab0 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 15:02:51 +0200 Subject: [PATCH 26/52] [skip ci] fix missing return --- src/node_http2.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_http2.cc b/src/node_http2.cc index 411e2adaf2b6b3..3bf745f15af0b9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1735,6 +1735,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { if (!is_write_scheduled()) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); + return; } if (is_destroyed()) { From 0872c51b0d3710f3300b43ab22618045ded53aea Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 15:04:44 +0200 Subject: [PATCH 27/52] [skip ci] fix that return --- src/node_http2.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 3bf745f15af0b9..66a4d004f845f0 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1735,10 +1735,9 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { if (!is_write_scheduled()) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); - return; } - if (is_destroyed()) { + if (!is_write_scheduled() && is_destroyed()) { HandleScope scope(env()->isolate()); MakeCallback(env()->ondone_string(), 0, nullptr); if (stream_ != nullptr) { From 8b757799704953a68e05c0f0c3ea710a77745f21 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 15:24:29 +0200 Subject: [PATCH 28/52] throw on frame error --- lib/internal/http2/core.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 8b282cb41b29e9..bcc356bb057ef2 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -613,10 +613,12 @@ function onFrameError(id, type, code) { return; debugSessionObj(session, 'error sending frame type %d on stream %d, code: %d', type, id, code); - const emitter = session[kState].streams.get(id) || session; + const stream = session[kState].streams.get(id); + const emitter = stream || session; emitter[kUpdateTimer](); emitter.emit('frameError', type, code, id); - session[kState].streams.get(id).close(code); + stream[kState].destroyCode = code; + stream.destroy(new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code)); session.close(); } From 28c80e58c66c0cfa788cf6c5fedb33995e8e0477 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 16:38:31 +0200 Subject: [PATCH 29/52] fix a test --- test/parallel/test-http2-server-socket-destroy.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index f47ad412777731..1d00d9c4eaf40d 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -39,7 +39,11 @@ function onStream(stream) { message: 'Stream closed with error code NGHTTP2_CANCEL' })); - socket.destroy(); + // Do not use destroy() as it sends FIN on Linux. + // On macOS, it sends RST. + + // Always send RST. + socket.resetAndDestroy(); } server.listen(0); From 64a133728d37731b73724c38f63855a96432b366 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 18:11:26 +0200 Subject: [PATCH 30/52] fix frame error --- lib/internal/http2/core.js | 13 +++++++++---- .../test-h2-large-header-cause-client-to-hangup.js | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index bcc356bb057ef2..91840ad923e174 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -616,10 +616,15 @@ function onFrameError(id, type, code) { const stream = session[kState].streams.get(id); const emitter = stream || session; emitter[kUpdateTimer](); - emitter.emit('frameError', type, code, id); - stream[kState].destroyCode = code; - stream.destroy(new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code)); - session.close(); + + if (stream) { + stream.emit('frameError', type, code, id); + stream[kState].rstCode = code; + stream.destroy(new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code)); + } else { + session[kState].destroyCode = code; + session.destroy(new ERR_HTTP2_SESSION_ERROR(nameForErrorCode[code] || code)); + } } function onAltSvc(stream, origin, alt) { diff --git a/test/parallel/test-h2-large-header-cause-client-to-hangup.js b/test/parallel/test-h2-large-header-cause-client-to-hangup.js index eb18d9e9a20ae7..d74de85bd54890 100644 --- a/test/parallel/test-h2-large-header-cause-client-to-hangup.js +++ b/test/parallel/test-h2-large-header-cause-client-to-hangup.js @@ -30,6 +30,7 @@ server.listen(0, common.mustCall(() => { stream.on('close', common.mustCall(() => { assert.strictEqual(stream.rstCode, NGHTTP2_FRAME_SIZE_ERROR); clearTimeout(timer); + clientSession.close(); server.close(); })); From 23f3b8c331d49fe097d60a3c30d536d7ed919dc2 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 19:19:00 +0200 Subject: [PATCH 31/52] fix frame errors --- lib/internal/http2/core.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 91840ad923e174..b2521c2fa554ff 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -619,6 +619,7 @@ function onFrameError(id, type, code) { if (stream) { stream.emit('frameError', type, code, id); + stream[kState].errorSendingFrame = true; stream[kState].rstCode = code; stream.destroy(new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code)); } else { @@ -2009,6 +2010,7 @@ class Http2Stream extends Duplex { didRead: false, flags: STREAM_FLAGS_PENDING, rstCode: undefined, + errorSendingFrame: false, writeQueueSize: 0, trailersReady: false, endAfterHeaders: false, @@ -2339,7 +2341,7 @@ class Http2Stream extends Duplex { debugStream(id || 'pending', session[kType], 'destroying stream'); - if (!this.closed) { + if (!this.closed && !this[kState].errorSendingFrame) { const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; let goawayCode = sessionState.goawayCode; From 585dd245739f511ccf16c205cb0b86c60822943d Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 19:27:32 +0200 Subject: [PATCH 32/52] send rst on frame error if server --- lib/internal/http2/core.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index b2521c2fa554ff..11d6a946872573 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2338,12 +2338,11 @@ class Http2Stream extends Duplex { const handle = this[kHandle]; const hasHandle = handle !== undefined; const sessionState = session[kState]; + const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; debugStream(id || 'pending', session[kType], 'destroying stream'); - if (!this.closed && !this[kState].errorSendingFrame) { - const isServerSession = session[kType] === NGHTTP2_SESSION_SERVER; - + if (!this.closed && (!this[kState].errorSendingFrame || isServerSession)) { let goawayCode = sessionState.goawayCode; if (goawayCode === NGHTTP2_NO_ERROR) { // Received NO_ERROR (GOAWAY) from remote peer yet still got reset From 68ef526efe74bcd8eb869f7234f722c43653e399 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 19:39:11 +0200 Subject: [PATCH 33/52] fix that again --- lib/internal/http2/core.js | 2 +- test/parallel/test-http2-exceeds-server-trailer-size.js | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 11d6a946872573..44c25a7d17ea9c 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2342,7 +2342,7 @@ class Http2Stream extends Duplex { debugStream(id || 'pending', session[kType], 'destroying stream'); - if (!this.closed && (!this[kState].errorSendingFrame || isServerSession)) { + if (!this.closed && (!this[kState].errorSendingFrame || this.headersSent || isServerSession)) { let goawayCode = sessionState.goawayCode; if (goawayCode === NGHTTP2_NO_ERROR) { // Received NO_ERROR (GOAWAY) from remote peer yet still got reset diff --git a/test/parallel/test-http2-exceeds-server-trailer-size.js b/test/parallel/test-http2-exceeds-server-trailer-size.js index ae2bbc1dca08bf..fae0c25096d8dc 100644 --- a/test/parallel/test-http2-exceeds-server-trailer-size.js +++ b/test/parallel/test-http2-exceeds-server-trailer-size.js @@ -42,7 +42,9 @@ server.listen(0, () => { const clientStream = clientSession.request(); - clientStream.on('close', common.mustCall()); + clientStream.on('close', () => { + clientSession.close(); + }); clientStream.on('error', common.expectsError({ code: 'ERR_HTTP2_STREAM_ERROR', name: 'Error', From 384b09f1969a5f9d56fe93b7ee26d228c6e59105 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 30 May 2024 19:54:49 +0200 Subject: [PATCH 34/52] fix that again --- lib/internal/http2/core.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 44c25a7d17ea9c..f1b0f7b289f364 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2342,7 +2342,11 @@ class Http2Stream extends Duplex { debugStream(id || 'pending', session[kType], 'destroying stream'); - if (!this.closed && (!this[kState].errorSendingFrame || this.headersSent || isServerSession)) { + // Do not use this.headersSent instead of trailersReady. + // headersSent is true when it **tries** to send headers. + // It is true even though it fails. + if (!this.closed && + (!this[kState].errorSendingFrame || this[kState].trailersReady || isServerSession)) { let goawayCode = sessionState.goawayCode; if (goawayCode === NGHTTP2_NO_ERROR) { // Received NO_ERROR (GOAWAY) from remote peer yet still got reset From 8275e2efd8359c2b5989740ea675f1e3309fa66c Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Fri, 4 Oct 2024 08:08:15 +0200 Subject: [PATCH 35/52] Check if session_call_on_frame_received frees the stream in session_after_header_block_received --- deps/nghttp2/lib/nghttp2_session.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index 08efaf0f8e9bda..8b818c30a2680a 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -4186,6 +4186,11 @@ static int session_after_header_block_received(nghttp2_session *session) { return 0; } + /* on_frame might free the stream */ + if (!nghttp2_session_get_stream(session, frame->hd.stream_id)) { + return 0; + } + return session_end_stream_headers_received(session, frame, stream); } From 8fa77113cde0ad4221353c829b84642d510e44f5 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Fri, 4 Oct 2024 08:20:44 +0200 Subject: [PATCH 36/52] patch --- ..._call_on_frame_received-frees-the-st.patch | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 deps/nghttp2/patches/0001-Check-if-session_call_on_frame_received-frees-the-st.patch diff --git a/deps/nghttp2/patches/0001-Check-if-session_call_on_frame_received-frees-the-st.patch b/deps/nghttp2/patches/0001-Check-if-session_call_on_frame_received-frees-the-st.patch new file mode 100644 index 00000000000000..737a17c03e2684 --- /dev/null +++ b/deps/nghttp2/patches/0001-Check-if-session_call_on_frame_received-frees-the-st.patch @@ -0,0 +1,29 @@ +From 9581880841f5afa00075eb6089389c03015334ce Mon Sep 17 00:00:00 2001 +From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> +Date: Fri, 4 Oct 2024 08:08:15 +0200 +Subject: [PATCH] Check if session_call_on_frame_received frees the stream in + session_after_header_block_received + +--- + deps/nghttp2/lib/nghttp2_session.c | 5 +++++ + 1 file changed, 5 insertions(+) + +diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c +index 08efaf0f8e..8b818c30a2 100644 +--- a/deps/nghttp2/lib/nghttp2_session.c ++++ b/deps/nghttp2/lib/nghttp2_session.c +@@ -4186,6 +4186,11 @@ static int session_after_header_block_received(nghttp2_session *session) { + return 0; + } + ++ /* on_frame might free the stream */ ++ if (!nghttp2_session_get_stream(session, frame->hd.stream_id)) { ++ return 0; ++ } ++ + return session_end_stream_headers_received(session, frame, stream); + } + +-- +2.46.2 + From c59704aa1bea65587ac3d28b206f51941fd38d2a Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Fri, 4 Oct 2024 11:52:30 +0200 Subject: [PATCH 37/52] Allow RST_STREAM before GOAWAY --- deps/nghttp2/lib/nghttp2_session.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index 8b818c30a2680a..67d8b08d36844f 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -2521,9 +2521,6 @@ static int session_prep_frame(nghttp2_session *session, return 0; } case NGHTTP2_RST_STREAM: - if (session_is_closing(session)) { - return NGHTTP2_ERR_SESSION_CLOSING; - } nghttp2_frame_pack_rst_stream(&session->aob.framebufs, &frame->rst_stream); return 0; case NGHTTP2_SETTINGS: { From 1b3ad5949ec0e5d82cece638d866c6bc327d3bbb Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Fri, 4 Oct 2024 12:15:32 +0200 Subject: [PATCH 38/52] fixup --- ...k-stream-free-after-header-received.patch} | 0 .../patches/0002-rst-before-goaway.patch | 26 +++++++++++++++++++ src/node_http2.cc | 9 ++----- 3 files changed, 28 insertions(+), 7 deletions(-) rename deps/nghttp2/patches/{0001-Check-if-session_call_on_frame_received-frees-the-st.patch => 0001-check-stream-free-after-header-received.patch} (100%) create mode 100644 deps/nghttp2/patches/0002-rst-before-goaway.patch diff --git a/deps/nghttp2/patches/0001-Check-if-session_call_on_frame_received-frees-the-st.patch b/deps/nghttp2/patches/0001-check-stream-free-after-header-received.patch similarity index 100% rename from deps/nghttp2/patches/0001-Check-if-session_call_on_frame_received-frees-the-st.patch rename to deps/nghttp2/patches/0001-check-stream-free-after-header-received.patch diff --git a/deps/nghttp2/patches/0002-rst-before-goaway.patch b/deps/nghttp2/patches/0002-rst-before-goaway.patch new file mode 100644 index 00000000000000..e4c2789899456e --- /dev/null +++ b/deps/nghttp2/patches/0002-rst-before-goaway.patch @@ -0,0 +1,26 @@ +From a990ac8598241f939ca41bf7a7c53c40094663bf Mon Sep 17 00:00:00 2001 +From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> +Date: Fri, 4 Oct 2024 11:52:30 +0200 +Subject: [PATCH] Allow RST_STREAM before GOAWAY + +--- + deps/nghttp2/lib/nghttp2_session.c | 3 --- + 1 file changed, 3 deletions(-) + +diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c +index 8b818c30a2..67d8b08d36 100644 +--- a/deps/nghttp2/lib/nghttp2_session.c ++++ b/deps/nghttp2/lib/nghttp2_session.c +@@ -2521,9 +2521,6 @@ static int session_prep_frame(nghttp2_session *session, + return 0; + } + case NGHTTP2_RST_STREAM: +- if (session_is_closing(session)) { +- return NGHTTP2_ERR_SESSION_CLOSING; +- } + nghttp2_frame_pack_rst_stream(&session->aob.framebufs, &frame->rst_stream); + return 0; + case NGHTTP2_SETTINGS: { +-- +2.46.2 + diff --git a/src/node_http2.cc b/src/node_http2.cc index 66a4d004f845f0..7f5b534ee662b8 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -798,13 +798,8 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // the peer but the HTTP/2 spec recommends sending it anyway. We'll // make a best effort. if (!socket_closed) { - Debug(this, "goaway session with code %d", code); - int32_t lastStreamID = - nghttp2_session_get_last_proc_stream_id(session_.get()); - CHECK_EQ( - nghttp2_submit_goaway( - session_.get(), NGHTTP2_FLAG_NONE, lastStreamID, code, nullptr, 0), - 0); + Debug(this, "terminating session with code %d", code); + CHECK_EQ(nghttp2_session_terminate_session(session_.get(), code), 0); SendPendingData(); } else if (stream_ != nullptr) { stream_->RemoveStreamListener(this); From 1d57327d9cbe98bde81f5c390e5db991201875cb Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 6 Oct 2024 13:33:24 +0200 Subject: [PATCH 39/52] fix --- lib/internal/http2/core.js | 12 +----------- lib/net.js | 8 ++++---- ...-http2-options-max-headers-exceeds-nghttp2.js | 16 +++++++--------- 3 files changed, 12 insertions(+), 24 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index f1b0f7b289f364..0dbf07313c0fed 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1151,17 +1151,7 @@ function finishSessionClose(session, error) { socket.resume(); } - // Always wait for writable side to finish. - socket.end((err) => { - debugSessionObj(session, 'finishSessionClose socket end', err, error); - // If session.destroy() was called, destroy the underlying socket. Delay - // it a bit to try to avoid ECONNRESET on Windows. - if (!session.closed) { - setImmediate(() => { - socket.destroy(error); - }); - } - }); + socket.resetAndDestroy(); } else { process.nextTick(emitClose, session, error); } diff --git a/lib/net.js b/lib/net.js index 604ba648754a2a..bdae5afdc17a23 100644 --- a/lib/net.js +++ b/lib/net.js @@ -732,8 +732,6 @@ Socket.prototype.end = function(data, encoding, callback) { Socket.prototype.resetAndDestroy = function() { if (this._handle) { - if (!(this._handle instanceof TCP)) - throw new ERR_INVALID_HANDLE_TYPE(); if (this.connecting) { debug('reset wait for connection'); this.once('connect', () => this._reset()); @@ -814,9 +812,11 @@ Socket.prototype._destroy = function(exception, cb) { this[kBytesRead] = this._handle.bytesRead; this[kBytesWritten] = this._handle.bytesWritten; - if (this.resetAndClosing) { + const tcpOrPipe = this._handle._parent ?? this._handle; + + if (this.resetAndClosing && tcpOrPipe instanceof TCP) { this.resetAndClosing = false; - const err = this._handle.reset(() => { + const err = tcpOrPipe.reset(() => { debug('emit close'); this.emit('close', isException); }); diff --git a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js index 3d651819af9cf0..a2cfdafded2cf6 100644 --- a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js +++ b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js @@ -22,12 +22,6 @@ server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`, options); - client.on('error', common.expectsError({ - code: 'ERR_HTTP2_SESSION_ERROR', - name: 'Error', - message: 'Session closed with error code NGHTTP2_COMPRESSION_ERROR' - })); - const req = client.request({ // Greater than 65536 bytes 'test-header': 'A'.repeat(90000) @@ -40,9 +34,9 @@ server.listen(0, common.mustCall(() => { })); req.on('error', common.expectsError({ - code: 'ERR_HTTP2_SESSION_ERROR', + code: 'ERR_HTTP2_STREAM_ERROR', name: 'Error', - message: 'Session closed with error code NGHTTP2_COMPRESSION_ERROR' + message: 'Stream closed with error code NGHTTP2_COMPRESSION_ERROR' })); req.end(); })); @@ -92,7 +86,11 @@ server.listen(0, common.mustCall(() => { const req = client.request(); req.on('response', common.mustNotCall()); - req.on('error', common.mustNotCall()); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_COMPRESSION_ERROR' + })); req.end(); }), ); From e78ba743fa9a17ad3b8fd8cf8a23be8a96560a1c Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 6 Oct 2024 13:53:06 +0200 Subject: [PATCH 40/52] fix --- lib/internal/http2/core.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 0dbf07313c0fed..fe781900ca49e0 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1145,13 +1145,13 @@ function finishSessionClose(session, error) { socket.on('close', () => { emitClose(session, error); }); - if (session.closed) { - // If we're gracefully closing the socket, call resume() so we can detect - // the peer closing in case binding.Http2Session is already gone. - socket.resume(); - } - socket.resetAndDestroy(); + // Always wait for writable side to finish. + socket.end((err) => { + debugSessionObj(session, 'finishSessionClose socket end', err, error); + socket.resetAndClosing = true; + socket.destroy(error); + }); } else { process.nextTick(emitClose, session, error); } From 0c3ac35cef219b659b7b72a8014d6e495e7f68c9 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 6 Oct 2024 15:58:39 +0200 Subject: [PATCH 41/52] Fix server close not being emitted if TLS socket RSTs --- lib/_tls_wrap.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 6818515d6ab684..b910c2348352cd 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -810,6 +810,10 @@ TLSSocket.prototype._init = function(socket, wrap) { const ssl = this._handle; this.server = options.server; + // Required for Socket.prototype._destroy check + // to emit 'close' server event + this._server = options.server; + debug('%s _init', options.isServer ? 'server' : 'client', 'handle?', !!ssl, From 9f96023f590bfb980b044588ae7c0576bb56a1f9 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Sun, 6 Oct 2024 18:43:10 +0200 Subject: [PATCH 42/52] fix --- lib/internal/http2/core.js | 3 +-- lib/net.js | 38 +++++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index fe781900ca49e0..2f00130ac8c4ce 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1149,8 +1149,7 @@ function finishSessionClose(session, error) { // Always wait for writable side to finish. socket.end((err) => { debugSessionObj(session, 'finishSessionClose socket end', err, error); - socket.resetAndClosing = true; - socket.destroy(error); + socket.resetAndDestroy(error); }); } else { process.nextTick(emitClose, session, error); diff --git a/lib/net.js b/lib/net.js index bdae5afdc17a23..0c255c4945e1ac 100644 --- a/lib/net.js +++ b/lib/net.js @@ -101,7 +101,6 @@ const { ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, ERR_INVALID_FD_TYPE, - ERR_INVALID_HANDLE_TYPE, ERR_INVALID_IP_ADDRESS, ERR_MISSING_ARGS, ERR_SERVER_ALREADY_LISTEN, @@ -730,13 +729,13 @@ Socket.prototype.end = function(data, encoding, callback) { return this; }; -Socket.prototype.resetAndDestroy = function() { +Socket.prototype.resetAndDestroy = function(error) { if (this._handle) { if (this.connecting) { debug('reset wait for connection'); - this.once('connect', () => this._reset()); + this.once('connect', () => this._reset(error)); } else { - this._reset(); + this._reset(error); } } else { this.destroy(new ERR_SOCKET_CLOSED()); @@ -812,11 +811,8 @@ Socket.prototype._destroy = function(exception, cb) { this[kBytesRead] = this._handle.bytesRead; this[kBytesWritten] = this._handle.bytesWritten; - const tcpOrPipe = this._handle._parent ?? this._handle; - - if (this.resetAndClosing && tcpOrPipe instanceof TCP) { - this.resetAndClosing = false; - const err = tcpOrPipe.reset(() => { + if (this.resetAndClosing && this._handle instanceof TCP) { + const err = this._handle.reset(() => { debug('emit close'); this.emit('close', isException); }); @@ -843,17 +839,29 @@ Socket.prototype._destroy = function(exception, cb) { if (this._server) { debug('has server'); - this._server._connections--; - if (this._server._emitCloseIfDrained) { - this._server._emitCloseIfDrained(); + + // This _destroy is called recursively on different parents + // till it closes all handles. + if (!this._parent) { + this._server._connections--; + + if (this._server._emitCloseIfDrained) { + this._server._emitCloseIfDrained(); + } } } }; -Socket.prototype._reset = function() { +Socket.prototype._reset = function(error) { debug('reset connection'); - this.resetAndClosing = true; - return this.destroy(); + + if (this._parentWrap) { + this._parentWrap.resetAndClosing = true; + } else { + this.resetAndClosing = true; + } + + return this.destroy(error); }; Socket.prototype._getpeername = function() { From 7c152dacae12742ef876877b73db37a7c8348bfc Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:15:18 +0200 Subject: [PATCH 43/52] does not matter if TCP FIN or TCP RST --- lib/internal/http2/core.js | 34 +++++++++++++------ ...st-http2-client-connection-tunnel-reset.js | 10 ++---- ...ttp2-respond-with-file-connection-abort.js | 6 +++- .../test-http2-server-socket-destroy.js | 13 ++----- 4 files changed, 35 insertions(+), 28 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 2f00130ac8c4ce..1340b87a06100d 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -3020,11 +3020,12 @@ ObjectDefineProperty(Http2Session.prototype, 'setTimeout', setTimeoutValue); function socketOnError(error) { const session = this[kBoundSession]; if (session !== undefined) { - // We can ignore ECONNRESET after GOAWAY was received as there's nothing - // we can do and the other side is fully within its rights to do so. - if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null) - return session.destroy(); - debugSessionObj(this, 'socket error [%s]', error.message); + if (error.code === 'ECONNRESET') { + // Handled by socketOnClose, it does not matter if the endpoint + // sends TCP FIN prematurely or TCP RST. + return; + } + debugSessionObj(session, 'socket error [%s]', error.message); session.destroy(error); } } @@ -3305,15 +3306,28 @@ function socketOnClose() { const err = session.connecting ? new ERR_SOCKET_CLOSED() : null; const state = session[kState]; + // Code for premature close let code = state.goawayCode ?? state.destroyCode; - if (code === NGHTTP2_NO_ERROR || code === null) { - code = session[kType] === NGHTTP2_SESSION_SERVER ? + if (code === NGHTTP2_NO_ERROR) { + // Got NO_ERROR but still closed prematurely + code = null; + } + + const defaultCode = session[kType] === NGHTTP2_SESSION_SERVER ? NGHTTP2_CANCEL : NGHTTP2_INTERNAL_ERROR; - } - state.streams.forEach((stream) => stream.close(code)); - state.pendingStreams.forEach((stream) => stream.close(code)); + const closeStream = (stream) => { + if (code === null) { + const connect = stream[kSentHeaders]?.[HTTP2_HEADER_METHOD] === HTTP2_METHOD_CONNECT; + stream.close(connect ? NGHTTP2_CONNECT_ERROR : defaultCode); + } else { + stream.close(code ?? defaultCode); + } + }; + + state.streams.forEach(closeStream); + state.pendingStreams.forEach(closeStream); session.close(); session[kMaybeDestroy](err); } diff --git a/test/parallel/test-http2-client-connection-tunnel-reset.js b/test/parallel/test-http2-client-connection-tunnel-reset.js index 83614525ae6256..6e19e3760aed05 100644 --- a/test/parallel/test-http2-client-connection-tunnel-reset.js +++ b/test/parallel/test-http2-client-connection-tunnel-reset.js @@ -20,18 +20,14 @@ h2Server.on('connect', (req, res) => { h2Server.listen(0, common.mustCall(() => { const proxyClient = h2.connect(`http://localhost:${h2Server.address().port}`); - proxyClient.once('error', common.expectsError({ - name: 'Error', - code: 'ECONNRESET', - message: 'read ECONNRESET' - })); + proxyClient.once('error', common.mustNotCall()); const proxyReq = proxyClient.request({ ':method': 'CONNECT', ':authority': 'example.com:443' }); proxyReq.once('error', common.expectsError({ name: 'Error', - code: 'ECONNRESET', - message: 'read ECONNRESET' + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_CONNECT_ERROR' })); })); diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index c2ad4e11a503a1..013560f0ce3d85 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/test/parallel/test-http2-respond-with-file-connection-abort.js @@ -13,7 +13,11 @@ const { const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { - stream.on('error', (err) => assert.strictEqual(err.code, 'ECONNRESET')); + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_CANCEL' + })); stream.respondWithFile(process.execPath, { [HTTP2_HEADER_CONTENT_TYPE]: 'application/octet-stream' }); diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index 1d00d9c4eaf40d..6f52436fde505a 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -39,9 +39,6 @@ function onStream(stream) { message: 'Stream closed with error code NGHTTP2_CANCEL' })); - // Do not use destroy() as it sends FIN on Linux. - // On macOS, it sends RST. - // Always send RST. socket.resetAndDestroy(); } @@ -50,18 +47,14 @@ server.listen(0); server.on('listening', common.mustCall(async () => { const client = h2.connect(`http://localhost:${server.address().port}`); - client.on('error', common.expectsError({ - name: 'Error', - code: 'ECONNRESET', - message: 'read ECONNRESET' - })); + client.on('error', common.mustNotCall()); client.on('close', common.mustCall()); const req = client.request({ ':method': 'POST' }); req.on('error', common.expectsError({ name: 'Error', - code: 'ECONNRESET', - message: 'read ECONNRESET' + code: 'ERR_HTTP2_STREAM_ERROR', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' })); req.on('aborted', common.mustCall()); From 44969885510596ff92bed84c101063d595a496ae Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:32:57 +0200 Subject: [PATCH 44/52] default to NGHTTP2_INTERNAL_ERROR --- lib/internal/http2/core.js | 4 +--- .../parallel/test-http2-respond-with-file-connection-abort.js | 2 +- test/parallel/test-http2-server-socket-destroy.js | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 1340b87a06100d..f2dd25e9856198 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -3313,9 +3313,7 @@ function socketOnClose() { code = null; } - const defaultCode = session[kType] === NGHTTP2_SESSION_SERVER ? - NGHTTP2_CANCEL : - NGHTTP2_INTERNAL_ERROR; + const defaultCode = NGHTTP2_INTERNAL_ERROR; const closeStream = (stream) => { if (code === null) { diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index 013560f0ce3d85..0f8993b8e43767 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/test/parallel/test-http2-respond-with-file-connection-abort.js @@ -16,7 +16,7 @@ server.on('stream', common.mustCall((stream) => { stream.on('error', common.expectsError({ code: 'ERR_HTTP2_STREAM_ERROR', name: 'Error', - message: 'Stream closed with error code NGHTTP2_CANCEL' + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' })); stream.respondWithFile(process.execPath, { [HTTP2_HEADER_CONTENT_TYPE]: 'application/octet-stream' diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index 6f52436fde505a..6b0ff60818143d 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -36,7 +36,7 @@ function onStream(stream) { stream.once('error', common.expectsError({ name: 'Error', code: 'ERR_HTTP2_STREAM_ERROR', - message: 'Stream closed with error code NGHTTP2_CANCEL' + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' })); // Always send RST. From 126fa33a30a3a78e443bbb848157a10f4a3fd0d8 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Thu, 10 Oct 2024 22:55:12 +0200 Subject: [PATCH 45/52] lint --- test/parallel/test-http2-respond-with-file-connection-abort.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index 0f8993b8e43767..8f06eb4a208054 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/test/parallel/test-http2-respond-with-file-connection-abort.js @@ -3,7 +3,6 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const assert = require('assert'); const http2 = require('http2'); const net = require('net'); From 4353b3fd60d33161dee3df1c1fafcebd8d071aee Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:22:17 +0200 Subject: [PATCH 46/52] fix flaky test-http2-respond-with-file-connection-abort.js --- ...ttp2-respond-with-file-connection-abort.js | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index 8f06eb4a208054..919d278b0cf254 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/test/parallel/test-http2-respond-with-file-connection-abort.js @@ -3,8 +3,16 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const { writeFile } = require('fs').promises; const http2 = require('http2'); const net = require('net'); +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const file = tmpdir.resolve('large-file'); + +const windowSize = 2 ** 16 - 1; +const largeBuffer = new Uint8Array(windowSize * 2); const { HTTP2_HEADER_CONTENT_TYPE @@ -17,24 +25,30 @@ server.on('stream', common.mustCall((stream) => { name: 'Error', message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' })); - stream.respondWithFile(process.execPath, { + // Do not use process.execPath because on shared builds it can be less than window size! + stream.respondWithFile(file, { [HTTP2_HEADER_CONTENT_TYPE]: 'application/octet-stream' }); })); -server.listen(0, common.mustCall(() => { - const client = http2.connect(`http://localhost:${server.address().port}`); - const req = client.request(); +(async () => { + await writeFile(file, largeBuffer); - req.once('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ERROR', - name: 'Error', - message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' - })); - req.on('response', common.mustCall()); - req.once('data', common.mustCall(() => { - net.Socket.prototype.destroy.call(client.socket); - server.close(); + server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + req.once('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' + })); + req.on('response', common.mustCall()); + req.once('data', common.mustCall(() => { + net.Socket.prototype.destroy.call(client.socket); + server.close(); + })); + //req.once('end', common.mustNotCall()); + req.end(); })); - req.end(); -})); +})(); From 246b623b9c24c98cd15faf92aa60a57e735af4f8 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:29:47 +0200 Subject: [PATCH 47/52] fix lint --- .../test-http2-respond-with-file-connection-abort.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index 919d278b0cf254..0220b7cc6ca70f 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/test/parallel/test-http2-respond-with-file-connection-abort.js @@ -37,7 +37,7 @@ server.on('stream', common.mustCall((stream) => { server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); - + req.once('error', common.expectsError({ code: 'ERR_HTTP2_STREAM_ERROR', name: 'Error', @@ -48,7 +48,7 @@ server.on('stream', common.mustCall((stream) => { net.Socket.prototype.destroy.call(client.socket); server.close(); })); - //req.once('end', common.mustNotCall()); + req.once('end', common.mustNotCall()); req.end(); })); -})(); +})().then(common.mustCall()); From 5ce564d927083234b2ae8f992b62ad8e67161d8f Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 15 Oct 2024 20:34:57 +0200 Subject: [PATCH 48/52] try to fix windows --- lib/internal/http2/core.js | 4 +--- test/parallel/test-http2-server-sessionerror.js | 12 ++++-------- test/parallel/test-http2-too-many-settings.js | 6 ++---- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index f2dd25e9856198..eefff3f1fb31e0 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -695,8 +695,6 @@ function onGoawayData(code, lastStreamID, buf) { session[kState].flags |= SESSION_FLAGS_CLOSED; session[kMaybeDestroy](); } else { - session[kState].flags |= SESSION_FLAGS_DESTROYED; - // Must not send any further data. closeSession( session, @@ -3326,7 +3324,7 @@ function socketOnClose() { state.streams.forEach(closeStream); state.pendingStreams.forEach(closeStream); - session.close(); + session[kState].flags |= SESSION_FLAGS_CLOSED; session[kMaybeDestroy](err); } } diff --git a/test/parallel/test-http2-server-sessionerror.js b/test/parallel/test-http2-server-sessionerror.js index 0800a6d7c35480..89af3e4c1826e4 100644 --- a/test/parallel/test-http2-server-sessionerror.js +++ b/test/parallel/test-http2-server-sessionerror.js @@ -45,19 +45,15 @@ server.listen(0, common.mustCall(() => { const url = `http://localhost:${server.address().port}`; http2.connect(url) .on('error', common.mustCall((err) => { - if (err.code !== 'ECONNRESET') { - assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); - assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); - } + assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); + assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); })) .on('close', () => { server.removeAllListeners('error'); http2.connect(url) .on('error', common.mustCall((err) => { - if (err.code !== 'ECONNRESET') { - assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); - assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); - } + assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); + assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); })) .on('close', () => server.close()); }); diff --git a/test/parallel/test-http2-too-many-settings.js b/test/parallel/test-http2-too-many-settings.js index 4a5697f3e63193..8094debc6c7867 100644 --- a/test/parallel/test-http2-too-many-settings.js +++ b/test/parallel/test-http2-too-many-settings.js @@ -30,10 +30,8 @@ function doTest(session) { server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`); client.on('error', common.mustCall((err) => { - if (err.code !== 'ECONNRESET') { - assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); - assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); - } + assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); + assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR'); })); client.on('close', common.mustCall(() => server.close())); })); From b9b31364e88fde47ea4f74132ae018a2de5a0fba Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 16 Oct 2024 01:56:54 +0200 Subject: [PATCH 49/52] fix moot condition --- lib/internal/http2/core.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index eefff3f1fb31e0..929d79ef20a42c 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2329,11 +2329,7 @@ class Http2Stream extends Duplex { debugStream(id || 'pending', session[kType], 'destroying stream'); - // Do not use this.headersSent instead of trailersReady. - // headersSent is true when it **tries** to send headers. - // It is true even though it fails. - if (!this.closed && - (!this[kState].errorSendingFrame || this[kState].trailersReady || isServerSession)) { + if (!this.closed) { let goawayCode = sessionState.goawayCode; if (goawayCode === NGHTTP2_NO_ERROR) { // Received NO_ERROR (GOAWAY) from remote peer yet still got reset From 58be6b5b7bd4b28d622b8340a8aa8d24c0824123 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 16 Oct 2024 02:00:03 +0200 Subject: [PATCH 50/52] remove moot errorSendingFrame --- lib/internal/http2/core.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 929d79ef20a42c..2d4a347892a094 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -619,7 +619,6 @@ function onFrameError(id, type, code) { if (stream) { stream.emit('frameError', type, code, id); - stream[kState].errorSendingFrame = true; stream[kState].rstCode = code; stream.destroy(new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code)); } else { @@ -1997,7 +1996,6 @@ class Http2Stream extends Duplex { didRead: false, flags: STREAM_FLAGS_PENDING, rstCode: undefined, - errorSendingFrame: false, writeQueueSize: 0, trailersReady: false, endAfterHeaders: false, From 56dea478d293a0a3f562dbefacddea9a4583f73d Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 16 Oct 2024 02:14:18 +0200 Subject: [PATCH 51/52] note --- lib/internal/http2/core.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 2d4a347892a094..25ca599abe6945 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2330,8 +2330,10 @@ class Http2Stream extends Duplex { if (!this.closed) { let goawayCode = sessionState.goawayCode; if (goawayCode === NGHTTP2_NO_ERROR) { - // Received NO_ERROR (GOAWAY) from remote peer yet still got reset - goawayCode = NGHTTP2_INTERNAL_ERROR; + // Received NO_ERROR (GOAWAY) from remote peer yet still got reset. + // This needs to be null because the other side is within its rights + // to TCP RST after we send non-zero GOAWAY. + goawayCode = null; } let destroyCode = sessionState.destroyCode; From 2f4d66f3421be4083b3328647f0d5bc27103808c Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 16 Oct 2024 03:45:51 +0200 Subject: [PATCH 52/52] quick fix --- lib/internal/http2/core.js | 19 ++++++++++--------- ...t-http2-client-rststream-before-connect.js | 6 ++++++ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 25ca599abe6945..5326b2d5627a31 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -547,15 +547,12 @@ function onStreamClose(code) { closeStream(stream, code, kNoRstStream); stream[kState].fd = -1; - // Defer destroy we actually emit end. - if (!stream.readable || code !== NGHTTP2_NO_ERROR) { - let error; - if (code !== NGHTTP2_NO_ERROR) { - error = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); - } - - // If errored or ended, we can destroy immediately. + if (code !== NGHTTP2_NO_ERROR) { + const error = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code); stream.destroy(error); + } else if (!stream.readable) { + // If errored or ended, we can destroy immediately. + stream.destroy(); } else { // Wait for end to destroy. stream.on('end', stream[kMaybeDestroy]); @@ -1875,7 +1872,11 @@ function afterShutdown(status) { const stream = this.handle[kOwner]; if (stream) { stream.on('finish', () => { - stream[kMaybeDestroy](); + // We don't want to interrupt in the middle + // where the stream is closing with an error code. + setImmediate(() => { + stream[kMaybeDestroy](); + }); }); } // Currently this status value is unused diff --git a/test/parallel/test-http2-client-rststream-before-connect.js b/test/parallel/test-http2-client-rststream-before-connect.js index ac9e6176d2bfb4..1ff56cf71ff440 100644 --- a/test/parallel/test-http2-client-rststream-before-connect.js +++ b/test/parallel/test-http2-client-rststream-before-connect.js @@ -61,6 +61,12 @@ server.listen(0, common.mustCall(() => { client.close(); })); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_PROTOCOL_ERROR' + })); + // The `response` event should not fire as the server should receive the // RST_STREAM frame before it ever has a chance to reply. req.on('response', common.mustNotCall());