From 1477f70dc059a4bdc4fdf5892671214bd6f93e1e Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 20 Oct 2026 07:02:27 -0700 Subject: [PATCH] http2: detect streams abandoned by nghttp2 When a peer sends GOAWAY(NO_ERROR), the session starts a graceful close on its own, which disarmed the detection of nghttp2 internally terminating the session after a protocol error. Instead, also treat it as an internal termination when nghttp2 no longer wants to read or write while we still have open streams. Signed-off-by: Matteo Collina --- src/node_http2.cc | 20 ++- src/node_http2.h | 1 + ...nup-on-nghttp2-goaway-after-peer-goaway.js | 116 ++++++++++++++++++ 3 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-http2-session-cleanup-on-nghttp2-goaway-after-peer-goaway.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 86b09c1d4efb..6aa389536f1d 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -801,6 +801,18 @@ bool Http2Session::HasPendingData() const { return true; } +// nghttp2 signals that the connection should be dropped by having both +// want_read and want_write return 0. If that happens while we still track +// open streams, nghttp2 has terminated the session on its own (for example +// due to a protocol error from the peer) and those streams will never be +// completed. +bool Http2Session::HasStreamsAbandonedByNghttp2() const { + if (HasPendingData()) return false; + return std::ranges::any_of(streams_, [](const auto& entry) { + return !entry.second->is_closed() && !entry.second->is_destroyed(); + }); +} + void Http2Session::EmitStatistics() { if (!HasHttp2Observer(env())) [[likely]] { return; @@ -2069,9 +2081,11 @@ uint8_t Http2Session::SendPendingData() { MaybeStopReading(); - // If nghttp2 has internally torn down the session (detected in OnFrameSent) - // during the nghttp2_session_mem_send loop above, at this point we error: - if (internal_goaway_sent_) { + // If nghttp2 has internally torn down the session (detected in OnFrameSent, + // or because it no longer wants to read or write while we still have open + // streams) during the nghttp2_session_mem_send loop above, at this point we + // error: + if (internal_goaway_sent_ || HasStreamsAbandonedByNghttp2()) { internal_goaway_sent_ = false; if (!is_closing() && !is_destroyed()) { Isolate* isolate = env()->isolate(); diff --git a/src/node_http2.h b/src/node_http2.h index c042c46ab595..f3e77d0a2faa 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -794,6 +794,7 @@ class Http2Session : public AsyncWrap, BaseObjectPtr PopPing(); bool AddPing(const uint8_t* data, v8::Local callback); bool HasPendingData() const; + bool HasStreamsAbandonedByNghttp2() const; BaseObjectPtr PopSettings(); bool AddSettings(v8::Local callback); diff --git a/test/parallel/test-http2-session-cleanup-on-nghttp2-goaway-after-peer-goaway.js b/test/parallel/test-http2-session-cleanup-on-nghttp2-goaway-after-peer-goaway.js new file mode 100644 index 000000000000..b249ada325e7 --- /dev/null +++ b/test/parallel/test-http2-session-cleanup-on-nghttp2-goaway-after-peer-goaway.js @@ -0,0 +1,116 @@ +'use strict'; + +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const net = require('net'); + +// When a peer sends GOAWAY(NO_ERROR), the session starts a graceful close +// on its own. If nghttp2 later terminates the session internally because of +// a protocol error (here, an oversized frame) while a stream is still open, +// the session must still be torn down and the error surfaced, rather than +// leaving the stream and connection hanging. + +const server = http2.createServer(); + +server.on('stream', common.mustCall((stream) => { + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_ERROR', + name: 'Error', + message: 'Protocol error' + })); + stream.on('close', common.mustCall()); +})); + +server.on('session', common.mustCall((session) => { + session.on('goaway', common.mustCall()); + session.on('error', common.expectsError({ + code: 'ERR_HTTP2_ERROR', + name: 'Error', + message: 'Protocol error' + })); + + session.on('close', common.mustCall(() => server.close())); +})); + +server.listen(0, common.mustCall(() => { + const conn = net.connect({ + port: server.address().port, + allowHalfOpen: true, + }); + + // HTTP/2 client connection preface. + conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'); + + // Empty SETTINGS frame. + const settingsFrame = Buffer.alloc(9); + settingsFrame[3] = 0x04; // type: SETTINGS + conn.write(settingsFrame); + + let inbuf = Buffer.alloc(0); + let state = 'settingsHeader'; + let settingsFrameLength; + + conn.on('data', (chunk) => { + inbuf = Buffer.concat([inbuf, chunk]); + + switch (state) { + case 'settingsHeader': + if (inbuf.length < 9) return; + settingsFrameLength = inbuf.readUIntBE(0, 3); + inbuf = inbuf.slice(9); + state = 'readingSettings'; + // Fallthrough + case 'readingSettings': { + if (inbuf.length < settingsFrameLength) return; + inbuf = inbuf.slice(settingsFrameLength); + state = 'done'; + + // ACK the server SETTINGS. + const ack = Buffer.alloc(9); + ack[3] = 0x04; // type: SETTINGS + ack[4] = 0x01; // flag: ACK + conn.write(ack); + + // Open stream 1 without END_STREAM, so it stays open on the server. + // HPACK: :method GET (0x82), :scheme http (0x86), :path / (0x84), + // :authority localhost (literal with indexed name 1). + const block = Buffer.from([ + 0x82, 0x86, 0x84, 0x41, 0x09, + ...Buffer.from('localhost'), + ]); + const headers = Buffer.alloc(9); + headers.writeUIntBE(block.length, 0, 3); + headers[3] = 0x01; // type: HEADERS + headers[4] = 0x04; // flags: END_HEADERS + headers.writeUInt32BE(1, 5); // stream id: 1 + conn.write(Buffer.concat([headers, block])); + + // GOAWAY(NO_ERROR), last stream id 0. This makes the server + // gracefully close its session without any application code. + const goaway = Buffer.alloc(17); + goaway.writeUIntBE(8, 0, 3); // length: 8 + goaway[3] = 0x07; // type: GOAWAY + goaway.writeUInt32BE(0, 9); // last stream id + goaway.writeUInt32BE(0, 13); // error code: NO_ERROR + conn.write(goaway); + + // Send a DATA frame header claiming length 16385, which exceeds the + // default max_frame_size of 16384, so nghttp2 calls + // nghttp2_session_terminate_session(FRAME_SIZE_ERROR) internally. + const oversized = Buffer.alloc(9); + oversized.writeUIntBE(16385, 0, 3); // length: 16385 (one over max) + oversized[3] = 0x00; // type: DATA + oversized.writeUInt32BE(1, 5); // stream id: 1 + conn.write(oversized); + } + } + }); + + // The server must close the connection after sending GOAWAY: + conn.on('end', common.mustCall(() => conn.end())); + conn.on('close', common.mustCall()); +}));