Fix: Discard an unconsumed request body when the response completes - #4272
Open
wwbakker wants to merge 3 commits into
Open
Fix: Discard an unconsumed request body when the response completes#4272wwbakker wants to merge 3 commits into
wwbakker wants to merge 3 commits into
Conversation
With request streaming, reading is driven by the consumer of the body: the channel is put in autoRead = false and the next read is only issued when the next chunk is asked for. A handler that answers without consuming the body to its end - one that rejects a request before decoding it, an unmatched route, a handler that ignores the body, or a reader that gives up half way - therefore leaves the connection open but never read from again, and the next request the client sends over it is silently dropped. HTTP/1.1 gives a server no way to ask a client to stop sending, so the two options once a response has been written early are to read the rest and discard it, or to close the connection. This does the former, up to maxDiscardedRequestBodySize, and the latter beyond it.
wwbakker
requested review from
987Nabil,
jdegoes,
kyri-petrou and
vigoo
as code owners
August 25, 2026 14:12
✅ Deploy Preview for zio-http ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses an HTTP/1.1 keep-alive correctness issue when RequestStreaming.Enabled is used and a handler completes a response without fully consuming the request body, which can leave the connection in a non-reading state and cause the next request on the same connection to be silently dropped.
Changes:
- Introduces
Server.Config.maxDiscardedRequestBodySizeto bound how much unconsumed request body the server will drain-and-discard before instead closing the connection. - Adds logic in the Netty server inbound path to trigger draining/discarding of any remaining request body once a response is produced.
- Adds a JVM regression spec that reproduces and verifies connection reusability (and close-on-overbudget behavior) using a raw socket split of headers/body.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
zio-http/shared/src/main/scala/zio/http/Server.scala |
Adds maxDiscardedRequestBodySize to server config, docs, and config loading. |
zio-http/jvm/src/main/scala/zio/http/netty/server/ServerInboundHandler.scala |
Captures the active AsyncBodyReader and triggers discard/drain after response handling to keep connections reusable. |
zio-http/jvm/src/main/scala/zio/http/netty/AsyncBodyReader.scala |
Adds a Discarding state and a method to drain-and-discard remaining inbound body (closing when budget exceeded). |
zio-http/jvm/src/test/scala/zio/http/netty/server/ServerUnconsumedRequestBodySpec.scala |
Adds regression coverage for unconsumed/partially-consumed bodies and over-budget close behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+99
to
+103
| // Captured here, on the event loop, while this request's reader is the one in the pipeline. Looking it up when the | ||
| // response completes would be a race: on a keep-alive connection the next request may have installed its own reader | ||
| // by then, and discarding its body would break a request that did nothing wrong. | ||
| val bodyReader = unfinishedBodyReader(ctx) | ||
| val done = () => { releaseRequest(); discardRemainingRequestBody(bodyReader) } |
Author
There was a problem hiding this comment.
Not sure what copilot means with this:
discardMainBody does not call ctx.close() as far as I can see.
And if I understand the code correctly (which granted, I might not), the discard/close logic is effectively chained to the 'future'/zio result by being used inside the attemptImmediateWrite clause, and/or being in the ensured parameter in the writeResponse.
Comment on lines
+153
to
+157
| case State.Discarding => | ||
| // The consumer is gone; read on until the body ends so that the connection stays usable, | ||
| // and give up on the connection if the client turns out to be sending more than we are | ||
| // willing to throw away. | ||
| discardableBytes -= content.length |
Author
There was a problem hiding this comment.
Updated content to be materialized only in the other branches.
Comment on lines
+149
to
+156
| test("a body too large to discard closes the connection instead of leaving it unreadable") { | ||
| for { | ||
| result <- scenario( | ||
| _ => ZIO.unit, | ||
| Server.Config.default.maxDiscardedRequestBodySize(bodySize.toLong / 2), | ||
| ).either | ||
| } yield assertTrue(result.isLeft) | ||
| }, |
Author
There was a problem hiding this comment.
Made the assertions more specific.
… tighten the over-budget test - channelRead0 materialised every chunk's bytes before the state match; a discarded chunk only needs its length, so the copy is now made lazily per branch. - Flush before closing when the discard budget is exceeded, so that a response still sitting in the outbound buffer is not dropped with the connection. - The over-budget test asserted only that the scenario failed, which would also pass if the response had never arrived. It now asserts that the rejection is delivered and that only the attempt to reuse the connection fails.
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.
Issue:
With request streaming, reading is driven by the consumer of the body: the channel is put in autoRead = false and the next read is only issued when the next chunk is asked for. A handler that answers without consuming the body to its end (i.e. one that rejects a request before decoding it, an unmatched route, a handler that ignores the body, or a reader that gives up half way) therefore leaves the connection open but never read from again, and the next request the client sends over it is silently dropped.
Possible solutions
HTTP/1.1 gives a server no way to ask a client to stop sending, so the two options once a response has been written early are:
Current PR
The changes in this PR do the former, up to maxDiscardedRequestBodySize, and the latter beyond it.
I personally think this is a nice solution, since it keeps the connection alive for smaller requests, but stops endlessly listening for larger ones, but it's definitely something you could have an different opinion about (and different use-cases might want different settings).
Also, the default size for maxDiscardedRequestBodySize is quite arbitrary. Someone with an opinion should make a judgement on that as well.
Context
It took a while to find the root cause of this issue when we ran into this issue in our own application, since the symptoms were flaky tests in our CI, which are obviously hard to diagnose. After having a reasonable idea where the issue could lie, I found the issue was already posted by someone else here:
#4193
I've used LLM tools with the diagnosing and the fix, but also had to use my own brain, since it alone wouldn't have found/fixed it.
I've also tested this fix in our own API and it did seem to fix the issue.