Commit f8ab6107a45 for nodejs
commit f8ab6107a45c9f0a7a859ab36dbf2ab00cd61122
Author: Yuya Inoue <65857152+inoway46@users.noreply.github.com>
Date: Sat Nov 14 14:16:17 2026 +0900
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 <inoueyuya416@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/67372
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 04acb5d30f6..d3695145115 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -348,6 +348,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;
@@ -644,6 +645,8 @@ function onStreamClose(code, peerReset) {
stream[kState].fd = -1;
if (locallyInitiated) {
+ // nghttp2 has already closed the stream, a deferred RST_STREAM is moot.
+ stream[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED;
stream.destroy();
return true;
}
@@ -2166,6 +2169,9 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) {
rstStreamStatus === kForceRstStream) {
finishCloseStream.call(stream, code);
} 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', finishCloseStream.bind(stream, code));
}
}
@@ -2188,6 +2194,7 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) {
}
function finishCloseStream(code) {
+ this[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED;
const rstStreamFn = submitRstStream.bind(this, code);
// Defer until the stream has an ID assigned.
if (this.pending) {
@@ -2681,8 +2688,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);
diff --git a/src/node_http2.cc b/src/node_http2.cc
index 2792199b449..f00a7a65c32 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -2930,7 +2930,10 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle,
Http2Session* session = static_cast<Http2Session*>(user_data);
Debug(session, "reading outbound data for stream %d", id);
BaseObjectPtr<Http2Stream> 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();
+}));