From f6b58cea9418d37e059f36a66586fd631feb3faf Mon Sep 17 00:00:00 2001 From: Peter Eisenlohr Date: Tue, 3 Nov 2026 15:46:29 +0000 Subject: [PATCH] h2: give back connection credit in the callback, as h2v2 does on_data_chunk_recv_callback() left that to NGHttp2Stream::on_data(), so DATA without a stream never got it back: with 1.35 MB of it forced through, the client ended the connection with FLOW_CONTROL_ERROR after 1 MB. Now the callback does it, with or without a stream. No peer can get there as it is. streams_ loses an entry only after nghttp2 has closed the stream, and then nghttp2 does not deliver the data, so there is no test for this: it would take a hook to remove a live stream from the session. It took a temporary patch to measure this, too. Both backends now have the same shape. For h2v2, that adds the start_write() its branch without a stream did not have, which could leave the WINDOW_UPDATE waiting for something else to wake up the send loop. on_data() of h2 lost the arguments it only needed for this, and now has the signature of the one of h2v2. Co-Authored-By: Claude Opus 5.5 --- include/anyhttp/h2/stream.hpp | 2 +- src/h2/session.cpp | 18 +++++++++++++----- src/h2/stream.cpp | 5 +---- src/h2v2/session.cpp | 25 ++++++++++++------------- 4 files changed, 27 insertions(+), 23 deletions(-) diff --git a/include/anyhttp/h2/stream.hpp b/include/anyhttp/h2/stream.hpp index 9a3af72..c9adec9 100644 --- a/include/anyhttp/h2/stream.hpp +++ b/include/anyhttp/h2/stream.hpp @@ -195,7 +195,7 @@ class NGHttp2Stream : public std::enable_shared_from_this ~NGHttp2Stream(); /// This function is called on receiving a data frame `nghttp2_on_data_chunk_recv_callback`. - void on_data(nghttp2_session* session, int32_t id_, const uint8_t* data, size_t len); + void on_data(const uint8_t* data, size_t len); /// This function is called on receiving a header or data frame that has the EOF flag set. void on_eof(nghttp2_session* session, int32_t id_); diff --git a/src/h2/session.cpp b/src/h2/session.cpp index 07034c3..e8d2ff5 100644 --- a/src/h2/session.cpp +++ b/src/h2/session.cpp @@ -373,15 +373,23 @@ static int on_data_chunk_recv_callback(nghttp2_session* session, uint8_t flags, auto handler = static_cast(user_data); auto stream = handler->find_stream(stream_id); - if (!stream) + // + // Connection-level credit is given back right away, with or without a stream: nobody else would, + // and the connection would run out of it. The stream-level window is only reopened once the + // reader has taken the data, which is what applies back pressure. + // + nghttp2_session_consume_connection(session, len); + + if (stream) { + logd("[{}.{}] on_data_chunk_recv_callback: DATA, len={}", handler->log_prefix(), stream_id, + len); + stream->on_data(data, len); + } + else logw("[{}.{}] on_data_chunk_recv_callback: DATA, but no stream found (id={})", handler->log_prefix(), stream_id, stream_id); - return 0; - } - logd("[{}.{}] on_data_chunk_recv_callback: DATA, len={}", handler->log_prefix(), stream_id, len); - stream->on_data(session, stream_id, data, len); handler->start_write(); // might re-open windows return 0; diff --git a/src/h2/stream.cpp b/src/h2/stream.cpp index 4571751..6fcbe7a 100644 --- a/src/h2/stream.cpp +++ b/src/h2/stream.cpp @@ -571,16 +571,13 @@ void NGHttp2Stream::call_read_handler(asio::const_buffer view) #endif } -void NGHttp2Stream::on_data(nghttp2_session* session, int32_t id_, const uint8_t* data, size_t len) +void NGHttp2Stream::on_data(const uint8_t* data, size_t len) { - assert(id == id_); assert(!eof_received); assert(len); // for EOF, on_eof() is called instead mlogd("read callback: {} bytes...", len); - nghttp2_session_consume_connection(session, len); - // // We try to invoke the read handler directly with the new buffer view. Only if there is no // pending read handler that can be invoked immediately, the data will be copied into a buffer. diff --git a/src/h2v2/session.cpp b/src/h2v2/session.cpp index c0a698f..8168d97 100644 --- a/src/h2v2/session.cpp +++ b/src/h2v2/session.cpp @@ -204,23 +204,22 @@ static int on_recv_data(nghttp2_conn* conn, int64_t stream_id, const uint8_t* da auto handler = static_cast(user_data); auto stream = static_cast(stream_user_data); - if (!stream) - { - logw("[{}] on_recv_data: DATA, but no stream found", handler->log_prefix(stream_id)); - nghttp2_conn_extend_max_offset(conn, datalen); - return 0; - } - - logd("[{}] on_recv_data: DATA, len={}", handler->log_prefix(stream_id), datalen); - // - // Connection-level credit is given back right away; the stream-level window is only reopened - // once the reader has taken the data, which is what applies back pressure. Version 1 split the - // same two steps between nghttp2_session_consume_connection() and consume_stream(). + // Connection-level credit is given back right away, with or without a stream: nobody else would, + // and the connection would run out of it. The stream-level window is only reopened once the + // reader has taken the data, which is what applies back pressure. Version 1 split the same two + // steps between nghttp2_session_consume_connection() and consume_stream(). // nghttp2_conn_extend_max_offset(conn, datalen); - stream->on_data(data, datalen); + if (stream) + { + logd("[{}] on_recv_data: DATA, len={}", handler->log_prefix(stream_id), datalen); + stream->on_data(data, datalen); + } + else + logw("[{}] on_recv_data: DATA, but no stream found", handler->log_prefix(stream_id)); + handler->start_write(); // might re-open windows return 0;