From aec66c0a7870f4a90c0392dc2841f5fffc7f4d74 Mon Sep 17 00:00:00 2001 From: Marten Richter Date: Sat, 4 Jul 2026 17:50:31 +0200 Subject: [PATCH 1/3] quic: correct http3 callback and fix revealed errs The http3 application had misinterpreted some of nghttp3 callbacks regarding stopSending and ResetStream. Actually, these callbacks asks the application to do the action and not informs about an event from the peer. The fixes lead to some failures of the automated tests, uncovering some problems: First headers, and pendingTrailers were reset, when the internal object went away, though the test wanted to read them. Second, during a graceful session shutdown, the implemented did not waited for all stream to be removed, but only one. Fixes: https://github.com/nodejs/node/issues/63657 Signed-off-by: Marten Richter --- lib/internal/quic/quic.js | 4 +-- src/quic/http3.cc | 26 +++++++++--------- src/quic/session.cc | 3 ++- src/quic/streams.cc | 56 ++++++++++++++++++++++----------------- src/quic/streams.h | 12 +++++++++ 5 files changed, 62 insertions(+), 39 deletions(-) diff --git a/lib/internal/quic/quic.js b/lib/internal/quic/quic.js index 99eeccc3c51d23..907f6037ae9194 100644 --- a/lib/internal/quic/quic.js +++ b/lib/internal/quic/quic.js @@ -2543,8 +2543,8 @@ class QuicStream { inner.ontrailers = undefined; inner.oninfo = undefined; inner.onwanttrailers = undefined; - inner.headers = undefined; - inner.pendingTrailers = undefined; + // do not reset headers here, this is still important information + // the same applies for pendingTrailers this.#handle = undefined; if (inner.fileHandle !== undefined) { // Close the FileHandle that was used as a body source. The close diff --git a/src/quic/http3.cc b/src/quic/http3.cc index bc479f96990577..8750bffe59daf0 100644 --- a/src/quic/http3.cc +++ b/src/quic/http3.cc @@ -921,24 +921,24 @@ class Http3ApplicationImpl final : public Session::Application { stream->ReceiveData(nullptr, 0, flags); } - void OnStopSending(stream_id id, error_code app_error_code) { + void OnSendStopSending(stream_id id, error_code app_error_code) { auto stream = session().FindStream(id); if (!stream) [[unlikely]] return; Debug(&session(), - "HTTP/3 application received stop sending for stream %" PRIi64, + "HTTP/3 application should send stop sending for stream %" PRIi64, id); - stream->ReceiveStopSending(QuicError::ForApplication(app_error_code)); + stream->SendStopSending(app_error_code); } - void OnResetStream(stream_id id, error_code app_error_code) { + void OnDoResetStream(stream_id id, error_code app_error_code) { auto stream = session().FindStream(id); if (!stream) [[unlikely]] return; Debug(&session(), - "HTTP/3 application received reset stream for stream %" PRIi64, + "HTTP/3 application received a request to reset stream for stream %" PRIi64, id); - stream->ReceiveStreamReset(0, QuicError::ForApplication(app_error_code)); + stream->DoStreamReset(app_error_code); } void OnShutdown(stream_id id) { @@ -1318,29 +1318,31 @@ class Http3ApplicationImpl final : public Session::Application { return NGTCP2_SUCCESS; } - static int on_stop_sending(nghttp3_conn* conn, + static int on_send_stop_sending(nghttp3_conn* conn, stream_id id, error_code app_error_code, void* conn_user_data, void* stream_user_data) { + // this callback asks the app side to send a stop sending NGHTTP3_CALLBACK_SCOPE(app); if (app.is_control_stream(id)) [[unlikely]] { return NGHTTP3_ERR_CALLBACK_FAILURE; } - app.OnStopSending(id, app_error_code); + app.OnSendStopSending(id, app_error_code); return NGTCP2_SUCCESS; } - static int on_reset_stream(nghttp3_conn* conn, + static int on_do_reset_stream(nghttp3_conn* conn, stream_id id, error_code app_error_code, void* conn_user_data, void* stream_user_data) { + // this callback ask the app side to do a reset stream NGHTTP3_CALLBACK_SCOPE(app); if (app.is_control_stream(id)) [[unlikely]] { return NGHTTP3_ERR_CALLBACK_FAILURE; } - app.OnResetStream(id, app_error_code); + app.OnDoResetStream(id, app_error_code); return NGTCP2_SUCCESS; } @@ -1394,9 +1396,9 @@ class Http3ApplicationImpl final : public Session::Application { on_begin_trailers, on_receive_trailer, on_end_trailers, - on_stop_sending, + on_send_stop_sending, on_end_stream, - on_reset_stream, + on_do_reset_stream, on_shutdown, nullptr, // recv_settings (deprecated) on_receive_origin, diff --git a/src/quic/session.cc b/src/quic/session.cc index 8380e477c01e80..e3fce1563e826d 100644 --- a/src/quic/session.cc +++ b/src/quic/session.cc @@ -2797,7 +2797,8 @@ void Session::RemoveStream(stream_id id) { // then we can proceed to finishing the close now. Note that the // expectation is that the session will be destroyed once FinishClose // returns. - if (impl_->state()->closing && impl_->state()->graceful_close) { + if (impl_->state()->closing && impl_->state()->graceful_close + && impl_->streams_.size() == 0) { FinishClose(); CHECK(is_destroyed()); } diff --git a/src/quic/streams.cc b/src/quic/streams.cc index e838392361f946..36f2667fcfcbd4 100644 --- a/src/quic/streams.cc +++ b/src/quic/streams.cc @@ -487,15 +487,7 @@ struct Stream::Impl { code = args[0].As()->Uint64Value(&unused); } - stream->EndReadable(); - - if (!stream->is_pending()) { - // If the stream is a local unidirectional there's nothing to do here. - if (stream->is_local_unidirectional()) return; - stream->NotifyReadableEnded(code); - } else { - stream->pending_close_read_code_ = code; - } + stream->SendStopSending(code); } // Sends a reset stream to the peer to tell it we will not be sending any @@ -512,21 +504,7 @@ struct Stream::Impl { code = args[0].As()->Uint64Value(&lossless); } - if (stream->state()->reset == 1) return; - - stream->EndWritable(); - // We can release our outbound here now. Since the stream is being reset - // on the ngtcp2 side, we do not need to keep any of the data around - // waiting for acknowledgement that will never come. - stream->outbound_.reset(); - stream->state()->reset = 1; - - if (!stream->is_pending()) { - if (stream->is_remote_unidirectional()) return; - stream->NotifyWritableEnded(code); - } else { - stream->pending_close_write_code_ = code; - } + stream->DoStreamReset(code); } JS_METHOD(SetPriority) { @@ -1823,6 +1801,36 @@ void Stream::ReceiveStreamReset(uint64_t final_size, QuicError error) { EmitReset(error); } +void Stream::DoStreamReset(error_code code) { + if (state()->reset == 1) return; + + EndWritable(); + // We can release our outbound here now. Since the stream is being reset + // on the ngtcp2 side, we do not need to keep any of the data around + // waiting for acknowledgement that will never come. + outbound_.reset(); + state()->reset = 1; + + if (!is_pending()) { + if (is_remote_unidirectional()) return; + NotifyWritableEnded(code); + } else { + pending_close_write_code_ = code; + } +} + +void Stream::SendStopSending(error_code code) { + EndReadable(); + + if (!is_pending()) { + // If the stream is a local unidirectional there's nothing to do here. + if (is_local_unidirectional()) return; + NotifyReadableEnded(code); + } else { + pending_close_read_code_ = code; + } +} + // ============================================================================ void Stream::EmitBlocked() { diff --git a/src/quic/streams.h b/src/quic/streams.h index 86cb36b2668985..8a5092d2cd350d 100644 --- a/src/quic/streams.h +++ b/src/quic/streams.h @@ -341,6 +341,18 @@ class Stream final : public AsyncWrap, void ReceiveStopSending(QuicError error); void ReceiveStreamReset(uint64_t final_size, QuicError error); + // Sends a reset stream to the peer to tell it we will not be sending any + // more data for this stream. This has the effect of shutting down the + // writable side of the stream for this peer. Any data that is held in the + // outbound queue will be dropped. The stream may still be readable. + void DoStreamReset(error_code code); + + // Tells the peer to stop sending data for this stream. This has the effect + // of shutting down the readable side of the stream for this peer. Any data + // that has already been received is still readable. + void SendStopSending(error_code code); + + // Currently, only HTTP/3 streams support headers. These methods are here // to support that. They are not used when using any other QUIC application. From c1c5b3f20c8391af85bcc6017124ef16584c76b2 Mon Sep 17 00:00:00 2001 From: Marten Richter Date: Sat, 4 Jul 2026 18:16:12 +0200 Subject: [PATCH 2/3] Quic: Fix linting --- lib/internal/quic/quic.js | 2 +- src/quic/http3.cc | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/internal/quic/quic.js b/lib/internal/quic/quic.js index 907f6037ae9194..2f4c9291264ad2 100644 --- a/lib/internal/quic/quic.js +++ b/lib/internal/quic/quic.js @@ -2543,7 +2543,7 @@ class QuicStream { inner.ontrailers = undefined; inner.oninfo = undefined; inner.onwanttrailers = undefined; - // do not reset headers here, this is still important information + // Do not reset headers here, this is still important information // the same applies for pendingTrailers this.#handle = undefined; if (inner.fileHandle !== undefined) { diff --git a/src/quic/http3.cc b/src/quic/http3.cc index 8750bffe59daf0..60d2926d97a7cc 100644 --- a/src/quic/http3.cc +++ b/src/quic/http3.cc @@ -936,7 +936,8 @@ class Http3ApplicationImpl final : public Session::Application { if (!stream) [[unlikely]] return; Debug(&session(), - "HTTP/3 application received a request to reset stream for stream %" PRIi64, + "HTTP/3 application received a request to reset stream for stream " + "%" PRIi64, id); stream->DoStreamReset(app_error_code); } From 0190b7587f055662967ce877c3e91f941405fae5 Mon Sep 17 00:00:00 2001 From: Marten Richter Date: Sat, 4 Jul 2026 18:41:07 +0200 Subject: [PATCH 3/3] quic: Fix lint2 --- src/quic/http3.cc | 16 ++++++++-------- src/quic/session.cc | 4 ++-- src/quic/streams.cc | 2 +- src/quic/streams.h | 1 - 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/quic/http3.cc b/src/quic/http3.cc index 60d2926d97a7cc..ce2f49b4bfb4c4 100644 --- a/src/quic/http3.cc +++ b/src/quic/http3.cc @@ -1320,10 +1320,10 @@ class Http3ApplicationImpl final : public Session::Application { } static int on_send_stop_sending(nghttp3_conn* conn, - stream_id id, - error_code app_error_code, - void* conn_user_data, - void* stream_user_data) { + stream_id id, + error_code app_error_code, + void* conn_user_data, + void* stream_user_data) { // this callback asks the app side to send a stop sending NGHTTP3_CALLBACK_SCOPE(app); if (app.is_control_stream(id)) [[unlikely]] { @@ -1334,10 +1334,10 @@ class Http3ApplicationImpl final : public Session::Application { } static int on_do_reset_stream(nghttp3_conn* conn, - stream_id id, - error_code app_error_code, - void* conn_user_data, - void* stream_user_data) { + stream_id id, + error_code app_error_code, + void* conn_user_data, + void* stream_user_data) { // this callback ask the app side to do a reset stream NGHTTP3_CALLBACK_SCOPE(app); if (app.is_control_stream(id)) [[unlikely]] { diff --git a/src/quic/session.cc b/src/quic/session.cc index e3fce1563e826d..15f2bcaf6d27ac 100644 --- a/src/quic/session.cc +++ b/src/quic/session.cc @@ -2797,8 +2797,8 @@ void Session::RemoveStream(stream_id id) { // then we can proceed to finishing the close now. Note that the // expectation is that the session will be destroyed once FinishClose // returns. - if (impl_->state()->closing && impl_->state()->graceful_close - && impl_->streams_.size() == 0) { + if (impl_->state()->closing && impl_->state()->graceful_close && + impl_->streams_.size() == 0) { FinishClose(); CHECK(is_destroyed()); } diff --git a/src/quic/streams.cc b/src/quic/streams.cc index 36f2667fcfcbd4..237a38071fd10a 100644 --- a/src/quic/streams.cc +++ b/src/quic/streams.cc @@ -1825,7 +1825,7 @@ void Stream::SendStopSending(error_code code) { if (!is_pending()) { // If the stream is a local unidirectional there's nothing to do here. if (is_local_unidirectional()) return; - NotifyReadableEnded(code); + NotifyReadableEnded(code); } else { pending_close_read_code_ = code; } diff --git a/src/quic/streams.h b/src/quic/streams.h index 8a5092d2cd350d..f1a040bb3687af 100644 --- a/src/quic/streams.h +++ b/src/quic/streams.h @@ -352,7 +352,6 @@ class Stream final : public AsyncWrap, // that has already been received is still readable. void SendStopSending(error_code code); - // Currently, only HTTP/3 streams support headers. These methods are here // to support that. They are not used when using any other QUIC application.