From 00c3cb4ef083da81022c3c41b754203e5ee041b5 Mon Sep 17 00:00:00 2001 From: inoway46 Date: Tue, 10 Nov 2026 13:06:14 +0900 Subject: [PATCH 1/2] http2: send deferred RST_STREAM on stream destroy close() with NO_ERROR waits for 'finish' before sending RST_STREAM. If the body is stuck behind flow control and the stream is destroyed first, RST_STREAM is never sent and nghttp2 keeps the stream open. On the next WINDOW_UPDATE, OnRead finds no stream and returns 0 without EOF, so nghttp2 sends empty DATA frames until OOM. Both client and server streams are affected. Send the deferred RST_STREAM in _destroy(). As a backstop, return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE from OnRead when the stream is gone, which nghttp2 documents as the way to reset a stream on error. Fixes: https://github.com/nodejs/node/issues/67363 Signed-off-by: inoway46 --- lib/internal/http2/core.js | 22 ++++++-- src/node_http2.cc | 5 +- .../test-http2-close-destroy-queued-body.js | 46 +++++++++++++++++ ...-http2-server-close-destroy-queued-body.js | 51 +++++++++++++++++++ 4 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http2-close-destroy-queued-body.js create mode 100644 test/parallel/test-http2-server-close-destroy-queued-body.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index a6077abb063..acb554002f0 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -340,6 +340,7 @@ const STREAM_FLAGS_HEADERS_SENT = 0x4; const STREAM_FLAGS_HEAD_REQUEST = 0x8; const STREAM_FLAGS_ABORTED = 0x10; const STREAM_FLAGS_HAS_TRAILERS = 0x20; +const STREAM_FLAGS_RST_DEFERRED = 0x40; const SESSION_FLAGS_PENDING = 0x0; const SESSION_FLAGS_READY = 0x1; @@ -616,6 +617,8 @@ function onStreamClose(code, peerReset) { closeStream(stream, code, kNoRstStream); stream[kState].fd = -1; + // nghttp2 has already closed the stream, a deferred RST_STREAM is moot. + stream[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED; // Defer destroy we actually emit end. if (!stream.readable || code !== NGHTTP2_NO_ERROR) { // If errored or ended, we can destroy immediately. @@ -2093,10 +2096,14 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) { if (rstStreamStatus !== kNoRstStream) { const finishFn = finishCloseStream.bind(stream, code); if (!ending || stream.writableFinished || code !== NGHTTP2_NO_ERROR || - rstStreamStatus === kForceRstStream) + rstStreamStatus === kForceRstStream) { finishFn(); - else + } else { + // Remember that the RST_STREAM is still owed, so that destroying the + // stream before 'finish' can submit it. See Http2Stream#_destroy(). + state.flags |= STREAM_FLAGS_RST_DEFERRED; stream.once('finish', finishFn); + } } if (type === NGHTTP2_SESSION_CLIENT) { @@ -2109,6 +2116,7 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) { } function finishCloseStream(code) { + this[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED; const rstStreamFn = submitRstStream.bind(this, code); // If the handle has not yet been assigned, queue up the request to // ensure that the RST_STREAM frame is sent after the stream ID has @@ -2520,8 +2528,16 @@ class Http2Stream extends Duplex { } const hasHandle = handle !== undefined; - if (!this.closed) + if (!this.closed) { closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream); + } else if (state.flags & STREAM_FLAGS_RST_DEFERRED) { + // close() deferred the RST_STREAM until 'finish', which will never be + // emitted now. Submit it here, otherwise nghttp2 keeps the stream open + // with a data provider that no longer has a stream to read from. + state.flags &= ~STREAM_FLAGS_RST_DEFERRED; + if (hasHandle) + handle.rstStream(code); + } this.push(null); if (hasHandle) { diff --git a/src/node_http2.cc b/src/node_http2.cc index eb0dc8e99c4..174d9e0c26a 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2886,7 +2886,10 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, Http2Session* session = static_cast(user_data); Debug(session, "reading outbound data for stream %d", id); BaseObjectPtr stream = session->FindStream(id); - if (!stream) return 0; + // The stream was destroyed without nghttp2 being told. Returning 0 here + // would make nghttp2 send empty DATA frames forever, so have it reset + // the stream instead. + if (!stream) return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; if (stream->statistics_.first_byte_sent == 0) stream->statistics_.first_byte_sent = uv_hrtime(); CHECK_EQ(id, stream->id()); diff --git a/test/parallel/test-http2-close-destroy-queued-body.js b/test/parallel/test-http2-close-destroy-queued-body.js new file mode 100644 index 00000000000..68cc846ffbc --- /dev/null +++ b/test/parallel/test-http2-close-destroy-queued-body.js @@ -0,0 +1,46 @@ +'use strict'; + +// Destroying a stream that was closed with NO_ERROR while its request body +// was still queued behind flow control must still send RST_STREAM. +// Refs: https://github.com/nodejs/node/issues/67363 + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const { NGHTTP2_NO_ERROR } = http2.constants; + +// A small stream window makes sure that most of the body stays queued. +const server = http2.createServer({ settings: { initialWindowSize: 16384 } }); + +let req; + +server.on('stream', common.mustCall((stream) => { + // The stream is not being read yet, so the request body is stuck behind + // flow control when the client closes and destroys the request. + req.close(); + req.destroy(); + + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ABORTED', + })); + stream.on('close', common.mustCall(() => { + assert.strictEqual(stream.rstCode, NGHTTP2_NO_ERROR); + server.close(); + })); + // Open the window again. Without the RST_STREAM this is what makes the + // client loop. + stream.resume(); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + req = client.request({ ':method': 'POST' }); + req.on('error', common.mustNotCall()); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.rstCode, NGHTTP2_NO_ERROR); + client.close(); + })); + req.end(Buffer.alloc(1024 * 1024)); +})); diff --git a/test/parallel/test-http2-server-close-destroy-queued-body.js b/test/parallel/test-http2-server-close-destroy-queued-body.js new file mode 100644 index 00000000000..635b8fe5241 --- /dev/null +++ b/test/parallel/test-http2-server-close-destroy-queued-body.js @@ -0,0 +1,51 @@ +'use strict'; + +// Destroying a stream that was closed with NO_ERROR while its response body +// was still queued behind flow control must still send RST_STREAM. +// Refs: https://github.com/nodejs/node/issues/67363 + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const { NGHTTP2_NO_ERROR } = http2.constants; + +const server = http2.createServer(); + +let serverStream; + +server.on('stream', common.mustCall((stream) => { + serverStream = stream; + stream.on('error', common.mustNotCall()); + stream.on('close', common.mustCall(() => { + assert.strictEqual(stream.rstCode, NGHTTP2_NO_ERROR); + })); + stream.respond(); + stream.end(Buffer.alloc(1024 * 1024)); +})); + +server.listen(0, common.mustCall(() => { + // A small stream window makes sure that most of the body stays queued. + const client = http2.connect(`http://localhost:${server.address().port}`, + { settings: { initialWindowSize: 16384 } }); + const req = client.request(); + req.on('response', common.mustCall(() => { + // The response is not being read yet, so the response body is stuck + // behind flow control when the server closes and destroys the stream. + serverStream.close(); + serverStream.destroy(); + // Open the window again. Without the RST_STREAM this is what makes the + // server loop. + req.resume(); + })); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ABORTED', + })); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.rstCode, NGHTTP2_NO_ERROR); + client.close(); + server.close(); + })); + req.end(); +})); From 6495fa9c91ae365047785c68ca8ec0cfe45601c4 Mon Sep 17 00:00:00 2001 From: gdemesa Date: Thu, 12 Nov 2026 14:33:31 -0800 Subject: [PATCH 2/2] fixup! http2: send deferred RST_STREAM on stream destroy On v24.x, a clean RST_STREAM received before the body ends closes the stream without an error, unlike main, where it errors with ERR_HTTP2_STREAM_ABORTED. Expect no error on the receiving side. The tests still run out of memory without the fix. Co-Authored-By: Claude Opus 5.5 Signed-off-by: gdemesa --- test/parallel/test-http2-close-destroy-queued-body.js | 6 +++--- .../parallel/test-http2-server-close-destroy-queued-body.js | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-http2-close-destroy-queued-body.js b/test/parallel/test-http2-close-destroy-queued-body.js index 68cc846ffbc..50f5710445e 100644 --- a/test/parallel/test-http2-close-destroy-queued-body.js +++ b/test/parallel/test-http2-close-destroy-queued-body.js @@ -22,9 +22,9 @@ server.on('stream', common.mustCall((stream) => { req.close(); req.destroy(); - stream.on('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ABORTED', - })); + // On this release line a clean reset before the body ends closes the + // stream without an error. + stream.on('error', common.mustNotCall()); stream.on('close', common.mustCall(() => { assert.strictEqual(stream.rstCode, NGHTTP2_NO_ERROR); server.close(); diff --git a/test/parallel/test-http2-server-close-destroy-queued-body.js b/test/parallel/test-http2-server-close-destroy-queued-body.js index 635b8fe5241..06e47c87009 100644 --- a/test/parallel/test-http2-server-close-destroy-queued-body.js +++ b/test/parallel/test-http2-server-close-destroy-queued-body.js @@ -39,9 +39,9 @@ server.listen(0, common.mustCall(() => { // server loop. req.resume(); })); - req.on('error', common.expectsError({ - code: 'ERR_HTTP2_STREAM_ABORTED', - })); + // On this release line a clean reset before the body ends closes the + // stream without an error. + req.on('error', common.mustNotCall()); req.on('close', common.mustCall(() => { assert.strictEqual(req.rstCode, NGHTTP2_NO_ERROR); client.close();