Use stream error for maxContentLength exceeded in InboundHttp2ToHttpAdapter#16558
Conversation
…th is exceeded InboundHttp2ToHttpAdapter threw a connection error (INTERNAL_ERROR) when maxContentLength was exceeded, causing a GOAWAY that closes the entire connection. This is incorrect — exceeding the content length for a single stream should only affect that stream. Changed to throw a stream error with ENHANCE_YOUR_CALM, which sends a RST_STREAM for the affected stream while keeping the connection open for other streams. Fixes netty#11994
…th is exceeded InboundHttp2ToHttpAdapter threw a connection error (INTERNAL_ERROR) when maxContentLength was exceeded, causing a GOAWAY that closes the entire connection. This is incorrect — exceeding the content length for a single stream should only affect that stream. Changed to throw a stream error with ENHANCE_YOUR_CALM, which sends a RST_STREAM for the affected stream while keeping the connection open for other streams. Fixes netty#11994
… is reverted - Changed boostrapEnv(0, 1, 1) to boostrapEnv(1, 1, 1) so clientLatch waits for the RST_STREAM exception on the client side. - Added awaitResponses() + assertions on clientException to verify: - The error is a StreamException (not connection error) - The stream ID is 3 (the oversized stream) - The error code is ENHANCE_YOUR_CALM - With the fix reverted (connectionError), the server sends GOAWAY instead of RST_STREAM, so InboundHttp2ToHttpAdapter.onRstStreamRead is never called, clientLatch never counts down, and awaitResponses() times out — correctly failing the test.
|
@chrisvest Thanks for the review! You're right — the previous test passed even with the fix reverted because it only checked whether stream 5 was delivered, which could succeed due to LocalChannel's frame-by-frame delivery semantics regardless of connection vs stream error. I've pushed a new commit that fixes this by asserting on the produced error directly: Key changes:
Why this fails when the fix is reverted: With With the fix ( |
| final int dataReadableBytes = data.readableBytes(); | ||
| if (content.readableBytes() > maxContentLength - dataReadableBytes) { | ||
| throw connectionError(INTERNAL_ERROR, | ||
| throw streamError(streamId, ENHANCE_YOUR_CALM, |
There was a problem hiding this comment.
I think stream error and this status code is appropriate. Typically this would be a 413 response in both HTTP/1.x and HTTP/2 (and then a RST would follow while a HTTP/1.x stream may try to salvage the connection or just close it) but I suspect it will end up surfacing only as a RST_STREAM.
There was a problem hiding this comment.
This is when the request body is too large to fully buffer? Yeah, that shouldn't be a connection error. In fact, ideally I'd expect a regular HTTP error code+RST_STREAM(NO_ERROR), but RST_STREAM alone would work okay.
Nothing in this class should be creating connection errors, right, since all essential protocol errors would have been noticed before getting here? Looking at the other instances:
throw connectionError(PROTOCOL_ERROR, "Data Frame received for unknown stream id %d", streamId);
throw connectionError(PROTOCOL_ERROR, "Push Promise Frame received for pre-existing stream id %d",
promisedStreamId);
Seems those can only happens if putMessage() had previously been called, and then PUSH_PROMISE when it passes allowAppend=false to processHeadersBegin() will return null? And that only happens if this was the second HEADERS for the stream. Definitely very strange, but does seem better as a stream error than connection error.
|
Please follow PR content conventions. Also, looking at the adapter, it seems weird to me that it has a content length limit at all. It should be possible to avoid the message copy and forward the body as-is. Downstream handlers can limit content length if they wish, but it shouldn't factor in here. But that may require an incompatible change. |
|
@yawkat Thanks for the feedback! I've updated the PR title and description to follow the content conventions. Regarding the broader question about whether Happy to discuss removing the limit entirely as a separate effort if that's desired. |
|
@ejona86 @yawkat @bryce-anderson so I am correct to say that you are ok with the change and we should merge it ? |
|
I have no concern. I didn't do a full review, but what I saw made it seem this is strictly better than the old behavior. |
…dapter (#16558) Motivation: When a message payload exceeds `maxContentLength` in `InboundHttp2ToHttpAdapter`, a connection error (`INTERNAL_ERROR`) is thrown. This sends a GOAWAY frame that closes the entire HTTP/2 connection, affecting all concurrent streams. Load balancers often treat GOAWAY as a 5xx error, even though this is a client-side issue (sending a payload that is too large). Exceeding the content length for a single stream should only affect that stream. Modification: Changed the error from `connectionError(INTERNAL_ERROR)` to `streamError(streamId, ENHANCE_YOUR_CALM)` in `onDataRead()`. The `ENHANCE_YOUR_CALM` error code is semantically appropriate for resource limit violations per RFC 7540 Section 7. Added test `exceedMaxContentLengthShouldCauseStreamErrorNotConnectionError()` that sends oversized data on stream 3 and verifies: - A `StreamException` (not connection error) is produced - The error targets stream 3 with `ENHANCE_YOUR_CALM` error code - The test correctly fails when the fix is reverted Result: Fixes #11994. (cherry picked from commit d8dd805)
|
Auto-port PR for 5.0: #16628 |
|
Auto-port PR for 4.2: #16629 |
…dapter (#16558) Motivation: When a message payload exceeds `maxContentLength` in `InboundHttp2ToHttpAdapter`, a connection error (`INTERNAL_ERROR`) is thrown. This sends a GOAWAY frame that closes the entire HTTP/2 connection, affecting all concurrent streams. Load balancers often treat GOAWAY as a 5xx error, even though this is a client-side issue (sending a payload that is too large). Exceeding the content length for a single stream should only affect that stream. Modification: Changed the error from `connectionError(INTERNAL_ERROR)` to `streamError(streamId, ENHANCE_YOUR_CALM)` in `onDataRead()`. The `ENHANCE_YOUR_CALM` error code is semantically appropriate for resource limit violations per RFC 7540 Section 7. Added test `exceedMaxContentLengthShouldCauseStreamErrorNotConnectionError()` that sends oversized data on stream 3 and verifies: - A `StreamException` (not connection error) is produced - The error targets stream 3 with `ENHANCE_YOUR_CALM` error code - The test correctly fails when the fix is reverted Result: Fixes #11994. (cherry picked from commit d8dd805)
bryce-anderson
left a comment
There was a problem hiding this comment.
Post merge 🚢 from me.
…undHttp2ToHttpAdapter (#16629) Auto-port of #16558 to 4.2 Cherry-picked commit: d8dd805 --- Motivation: When a message payload exceeds `maxContentLength` in `InboundHttp2ToHttpAdapter`, a connection error (`INTERNAL_ERROR`) is thrown. This sends a GOAWAY frame that closes the entire HTTP/2 connection, affecting all concurrent streams. Load balancers often treat GOAWAY as a 5xx error, even though this is a client-side issue (sending a payload that is too large). Exceeding the content length for a single stream should only affect that stream. Modification: Changed the error from `connectionError(INTERNAL_ERROR)` to `streamError(streamId, ENHANCE_YOUR_CALM)` in `onDataRead()`. The `ENHANCE_YOUR_CALM` error code is semantically appropriate for resource limit violations per RFC 7540 Section 7. Added test `exceedMaxContentLengthShouldCauseStreamErrorNotConnectionError()` that sends oversized data on stream 3 and verifies: - A `StreamException` (not connection error) is produced - The error targets stream 3 with `ENHANCE_YOUR_CALM` error code - The test correctly fails when the fix is reverted Result: Fixes #11994. Co-authored-by: Guimu <30684111+daguimu@users.noreply.github.com>
…undHttp2ToHttpAdapter (#16628) Auto-port of #16558 to 5.0 Cherry-picked commit: d8dd805 --- Motivation: When a message payload exceeds `maxContentLength` in `InboundHttp2ToHttpAdapter`, a connection error (`INTERNAL_ERROR`) is thrown. This sends a GOAWAY frame that closes the entire HTTP/2 connection, affecting all concurrent streams. Load balancers often treat GOAWAY as a 5xx error, even though this is a client-side issue (sending a payload that is too large). Exceeding the content length for a single stream should only affect that stream. Modification: Changed the error from `connectionError(INTERNAL_ERROR)` to `streamError(streamId, ENHANCE_YOUR_CALM)` in `onDataRead()`. The `ENHANCE_YOUR_CALM` error code is semantically appropriate for resource limit violations per RFC 7540 Section 7. Added test `exceedMaxContentLengthShouldCauseStreamErrorNotConnectionError()` that sends oversized data on stream 3 and verifies: - A `StreamException` (not connection error) is produced - The error targets stream 3 with `ENHANCE_YOUR_CALM` error code - The test correctly fails when the fix is reverted Result: Fixes #11994. Co-authored-by: Guimu <30684111+daguimu@users.noreply.github.com>
Motivation:
When a message payload exceeds
maxContentLengthinInboundHttp2ToHttpAdapter, a connection error (INTERNAL_ERROR) is thrown. This sends a GOAWAY frame that closes the entire HTTP/2 connection, affecting all concurrent streams. Load balancers often treat GOAWAY as a 5xx error, even though this is a client-side issue (sending a payload that is too large). Exceeding the content length for a single stream should only affect that stream.Modification:
Changed the error from
connectionError(INTERNAL_ERROR)tostreamError(streamId, ENHANCE_YOUR_CALM)inonDataRead(). TheENHANCE_YOUR_CALMerror code is semantically appropriate for resource limit violations per RFC 7540 Section 7.Added test
exceedMaxContentLengthShouldCauseStreamErrorNotConnectionError()that sends oversized data on stream 3 and verifies:StreamException(not connection error) is producedENHANCE_YOUR_CALMerror codeResult:
Fixes #11994.