Repository navigation
h2: give back connection credit in the callback, as h2v2 does - #34
Merged
Merged
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.