fix(h2): honour maxResponseSize - #5676
Open
ondraulehla wants to merge 1 commit into
Open
Conversation
Signed-off-by: Ondřej Úlehla <106835858+ondraulehla@users.noreply.github.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.
This relates to...
Nothing filed. I noticed it while reading the two client implementations side by side after #5638.
Rationale
maxResponseSizeis documented on the HTTP/2 client's own page,docs/docs/api/H2CClient.md:It is declared in
types/h2c-client.d.tsand validated inlib/dispatcher/client.js(anH2CClientwithmaxResponseSize: -5throwsInvalidArgumentError), then stored intoclient[kMaxResponseSize].lib/dispatcher/client-h2.jsnever reads that symbol, so the value is validated and then ignored.onData()forwards every chunk torequest.onResponseData(chunk)with no byte accounting.Server sends 4 MiB, client asks for
maxResponseSize: 1024:ClientH2CClientUND_ERR_RES_EXCEEDED_MAX_SIZEUND_ERR_RES_EXCEEDED_MAX_SIZEUND_ERR_RES_EXCEEDED_MAX_SIZESame split for a streaming consumer: over h1 the body errors mid-stream, over h2 it just completes.
The enforcement path itself isn't a regression, h2 has never honoured the option:
git log -S kMaxResponseSize -- lib/dispatcher/client-h2.jsis empty, and 7.29.0 ignores it too once you passallowH2: true.What did change is what an ordinary caller gets. Against a TLS origin offering h2, with h2 mentioned nowhere in the code:
new Agent({ maxResponseSize })UND_ERR_RES_EXCEEDED_MAX_SIZEconnect: { allowH2: false }UND_ERR_RES_EXCEEDED_MAX_SIZEUND_ERR_RES_EXCEEDED_MAX_SIZEThat's #4828 making
allowH2default totrue. Opting out of h2 brings the limit back, which is the clearest sign of where it goes missing.Changes
onData()counts bytes againstmaxResponseSize, mirroringonBody()inclient-h1.js. The counter and the limit live on the pre-shaped per-requeststateobject, so the hidden class is unchanged.One deliberate difference from h1, and I'd like a second opinion on it.
client-h1.jscallsutil.destroy(socket, ...)because HTTP/1.1 can't abandon one response without losing framing. Over h2 that would take out every sibling stream on the session, so this calls the existing per-requeststate.abort()instead, which resets only the offending stream. I verified the session survives: after an oversized stream is reset, later requests on the same client still succeed and the server reports one connection.Tests
Two cases added to
test/max-response-size.js, which was http/1.1 only. Both fail before the change (the oversized response resolves instead of throwing) and pass after:ResponseExceededMaxSizeErrortest/max-response-size.js: 6 pass.test/http2*.js test/h2c-client.js: 105 pass, 0 fail. The default (-1) path is unchanged, a 4 MiB body is still delivered in full with no limit set and withmaxResponseSize: -1.