Repository navigation
fix(netty): detach a replayed request's old channel before completing its body control - #2367
Merged
hyperxpro merged 1 commit intoNov 4, 2026
Conversation
Motivation: replayRequest completed the old response's body control before it detached the old channel from the future. On HTTP/2, completing the control resumes the suspended stream channel, which delivers the DATA frames queued in it on the spot. The channel still named the future, so the handler that had just been told to retry received parts of the abandoned response before the replayed response's status. On HTTP/1.1 with auto-read off, nothing read the old channel after the replay. The drain that replayRequest installs never saw the end of the response, so the connection was never drained or closed. Modification: Mark the HTTP/2 stream channel as discarded, or install the HTTP/1.1 drain callback, before completing the control. When auto-read is off, issue one read so that the drain starts. Result: After onRetry, the handler receives the replayed response, starting with its status, and the old HTTP/1.1 connection is drained and closed. The HTTP/2 test queues DATA frames in the suspended stream channel; the HTTP/1.1 test replays after an I/O error with auto-read off. Both fail without the change. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hyperxpro
approved these changes
Nov 4, 2026
4 tasks done
hyperxpro
pushed a commit
that referenced
this pull request
Nov 4, 2026
…re the replay (#2368) ## Summary - Fixes the flaky `ResponseBodyControlTest.ioExceptionReplayDrainsTheOldConnectionWithAutoReadDisabled`, added in #2367. It failed on macOS with JDK 21 in #2367's first CI run, and about once in ten runs locally. - Test only; the fix from #2367 is unchanged. ## Problem The test's server sends the first response's chunk right after its head. When both arrive in the same read, the handler receives the chunk before the injected I/O error replays the request. That is correct, since a suspension does not hold back what has already been read, but the test took the chunk for part of the replayed body (`expected: <replayed> but was: <first>`). Recording the order of events over 50 repetitions on JDK 11: 3 runs had the chunk before `onRetry`, none after it. ## Change The test records `onRetry`, allows the first response's chunk only before it, and checks that the replayed body follows it directly. What the test is about is unchanged: with auto-read off, the old connection must still be drained and closed. ## AI disclosure Claude Code on behalf of Matthias Kurz. The commit includes `Co-Authored-By` per `AGENTS.md`. ## Test plan - [x] The test repeated 50 times on JDK 11 and on JDK 25: 0 failures. The previous version failed 7 times on JDK 11 and once on JDK 25. - [x] Without #2367's change in `NettyRequestSender`, every one of 50 repetitions still fails, on the drain assertion. - [x] `./mvnw -B -ntp -Dgpg.skip=true clean verify` on JDK 11, on a tree identical to this branch: 1,821 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used. - [x] `ResponseBodyControlTest` and `Http2ResponseBodyControlTest`, compiled on JDK 11 and run on JDK 17, 21 and 25 (Maven Surefire's `-Djvm`): 20 tests each, 0 failures. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
14 tasks done
hyperxpro
added a commit
that referenced
this pull request
Nov 5, 2026
## Summary - While an HTTP/1.1 response is suspended through `ResponseBodyControl`, stop decompressing its body, for the content codings AHC decompresses: gzip, deflate, br, zstd and snappy. Body parts are at most 64 KiB, so a handler that suspends after each part receives one part per `resume()`. - Rebuilt on the inflate loop of #2357. `Http1ContentDecompressor` keeps its superclass and constructor, and also implements `ChannelOutboundHandler` to see read requests. Revapi needs no entries. - br, zstd and snappy are decompressed on demand too, with Netty's pull-based `Decompressor`, in a class of their own. Only these codings use that API, which Netty added in 4.2.17; without it they go to Netty's own decoders, as before. - A body that is being drained is no longer decompressed, and a failed decoder result on the last chunk is kept, for these content codings. - HTTP/2 decompression is unchanged; see below. The replay fix that used to be part of this pull request is now #2367. ## Problem Suspending a response stops further socket reads, but Netty's `HttpContentDecompressor`, and the inflate loop that replaces it for gzip and deflate since #2357, decompress every chunk completely as soon as it is read. So one socket read of a highly compressible body still became many parts that nobody had asked for. For example, one 64 KiB read of gzipped zeros (about 1032:1) becomes some 67 MB of body parts for a handler that has suspended the response. Only `maxDecompressedResponseSize` (256 MiB by default) bounds it. With 64 MiB of gzipped zeros and a handler that suspends inside each body callback, `main` before #2357 delivered 1,955,380 bytes in 30 more parts past the suspending part, and then up to 33,713,376 bytes per resume. This branch without its suspension check delivers the same 1,955,380 bytes in 30 parts. AHC 2 behaved the same way, so this is hardening, not a regression fix. It matters to consumers that bound their buffering through `ResponseBodyControl`, such as Play WS's streaming API. ## Change **gzip and deflate**, on #2357's loop: - **Before each part**, the handler checks whether the response is suspended. Parts are cut where `JdkZlibDecoder` cuts them, but each `inflate()` call is capped at what is left of a 64 KiB part, so no part is larger. - **While suspended**, the input the inflater has not used yet stays in the handler, together with every message that arrives after it. The chunk is retained, not copied, since the inflater may still point into it; a direct buffer stays allocated until the inflater has used it up. When the output buffer filled up, the inflater may also hold output without any input left; that is remembered, and handed on after the resume. - **On resume**, the handler intercepts the read request that `resume()` issues. It hands on what was held back first, and lets the read reach the socket only if the response is not suspended again. This works the same with auto-read on and off. While paused, `channelReadComplete` does not request a read. - **Without suspension**, each part goes straight through, as before. **br, zstd and snappy**: `PullDecompression` decompresses them one part at a time with Netty's `Decompressor`, under the same checks. Small outputs, such as snappy's frames, are merged into a part sized for them. Outputs larger than 64 KiB, such as zstd's when `io.netty.compression.defaultMaxForwardBytes` is raised, are split into retained slices, so the bound applies to delivered parts, not to allocations. Only this class refers to Netty's `Decompressor`, and it is loaded only once one of these codings shows up. Without the API, which Netty added in 4.2.17, these codings go to `HttpContentDecompressor`'s own decoders, as before. So does zstd when `io.netty.compression.defaultMaxForwardBytes` is 0 or less, where Netty's pull decompressor hands out no output. **For all five content codings**: - **End of the body:** only parts with content wait for a resume. Once the body has been decompressed in full, its end, with any trailers, is handed on even to a suspended response, as for an unencoded body. A consumer whose demand ended with the last part still learns that the body is complete. - **Connection close:** if the connection closes while input is held back, the rest is decompressed and delivered before `channelInactive`, even to a suspended response, because Netty tears the pipeline down with the channel. Suspension no longer bounds that output; only `maxDecompressedResponseSize` does. - **Bodies nobody reads:** once the exchange has ended (cancel, `ABORT`, timeout), or while the channel is drained (after a redirect, an authentication challenge or a replay), the rest of the body is dropped instead of decompressed. Its end still reaches a drain callback. - **The last chunk's decoder result:** `HttpObjectDecoder` reports a chunk it cannot parse as a `LastHttpContent` with a failed result. The end of the body now keeps that result, so the request fails instead of completing with a truncated body. A transfer coding such as `Transfer-Encoding: gzip` still goes through Netty's decoder, which drops it, as on `main`. A suspension made from another thread takes effect when its queued task runs. Until then, the event loop can keep reading and decompressing, as before. ### Why HTTP/2 decompression is unchanged After `END_STREAM`, Netty's HTTP/2 stream channel hands every queued frame over on the next read, regardless of `autoRead`, and then closes. So a handler in the stream pipeline cannot hold compressed input back, and would have to inflate up to a whole stream window at once (16 MiB by default). That is exactly the decompression-bomb case, so a partial fix would be misleading. It needs either a Netty change (honor demand after `END_STREAM` and close only when drained), proposed in netty/netty#17774, or AHC delivering the body independently of the stream channel. ## Behavior changes - **Suspended HTTP/1.1 responses** with one of these content codings receive no further part with content until they resume. A handler that suspends after each part receives one part of at most 64 KiB per resume. The empty last part, trailers and completion still arrive while suspended once the body has been decompressed in full. - **Connection close while suspended:** what the decompressor still holds is delivered at once, bounded only by `maxDecompressedResponseSize`, not by demand. This applies to any close that finds input held back, not only to bodies delimited by the close. - **Drains:** a body with one of these content codings is no longer decompressed while it is drained. Before, a body beyond `maxDecompressedResponseSize` failed the drain, closed its connection and could fail the request that followed on the same future, for example when a `ResponseFilter` replays it. Redirects and 401 or 407 challenges with a compressed body usually close the connection instead of draining it, because the decompressed response looks chunked to them, as on `main`. - **Parts** of these codings are at most 64 KiB. With AHC's default chunk size of 8 KiB, gzip and deflate parts were 64 KiB already; a larger chunk could inflate into a larger part before. For br, zstd and snappy, small outputs are now merged and large ones split, so their part counts differ from Netty's decoders. - **Framing errors** on the final chunk of a body in one of these content codings, such as a bad chunk size, now fail the request. Before, it completed with a truncated body. A transfer coding is unchanged. - **br, zstd and snappy wait for demand with Netty 4.2.17 or later.** With an older Netty, they are decompressed by Netty's own decoders as soon as they are read, as before. gzip and deflate do not use the API. - **Unchanged:** a transfer coding (`Transfer-Encoding: gzip`) is still decoded by Netty's `HttpContentDecompressor`, as on `main`: as soon as it is read, also while suspended or drained, and in Netty's parts. `ResponseBodyControl.suspend()` says so. After a failure, such as the size limit, what is left of the body is dropped, its end included, as on `main`. ## Compatibility - **API:** `Http1ContentDecompressor` keeps its superclass, `HttpContentDecompressor`, and its constructor. It now also implements `ChannelOutboundHandler`, and gains the public methods of that interface, which pass every outbound event through and only look at read requests. Revapi reports no problem. - **Netty:** AHC uses Netty 4.2.19. Only br, zstd and snappy use Netty's `Decompressor` API, added in 4.2.17, and fall back to Netty's own decoders without it. Netty still marks it as unstable (`@UnstableApi`) while it migrates its codecs to it ([netty/netty#16743](netty/netty#16743)), so a future Netty release may require changes to `PullDecompression`. The README says what the version buys. - **Performance:** not measured again for this revision. Without suspension, gzip and deflate run #2357's loop plus the checks before each part; br, zstd and snappy use Netty's pull decompressors instead of its stream decoders. ## Follow-ups - At the end of the body, check that the compressed stream is complete, so a body cut off in the middle of a gzip member, or missing its checksum, fails instead of completing. For gzip and deflate that means the inflater's state and the gzip trailer, for br, zstd and snappy the pull decoder's `endOfInput()`. That changes behavior `main` has, so it gets its own pull request. - Output that is still held when the connection closes, and its HTTP/2 counterpart: the output of the final DATA frame once the stream channel closes (netty/netty#17774 is a prerequisite for HTTP/2). They are related, but may need different fixes. ## Review fixes Four commits on top, one per review finding, each with a test that fails without it: inflate buffers are capped at the part size, so a large codec chunk no longer gives every 64 KiB part a buffer twice the chunk's size; br, zstd and snappy fall back to Netty's own decoders when the `Decompressor` API is missing instead of failing; zstd is left to Netty at a forwarding threshold of 0, where its pull decompressor hands out nothing and the loop never ended; and the README and Javadoc say when these codings wait for demand. ## AI disclosure Claude Code on behalf of Matthias Kurz. The commits include `Co-Authored-By` per `AGENTS.md`. ## Test plan New and changed tests: - **`Http1DecompressionSuspensionTest` (10 tests, live server):** the bound with auto-read on and off, connection reuse, a chunked body with trailers, a body ended by connection close, cancel, `ABORT`, `HEAD` and empty gzip responses, a replay while input is held back with auto-read on and off, and a `ResponseFilter` replay of a body that inflates far beyond the limit, whose drained connection must go back to the pool. - **`Http1ContentDecompressorSuspensionTest` (18 tests, `EmbeddedChannel`):** - one part per read, suspension before the body, and the end of a fully inflated body without another resume, also for a body without bytes; - close while input is held, removal releasing it, and the size limit on the resume path; - suspension across a gzip member boundary, and every coding with fragmented input and a resume per part, with auto-read on and off; - a direct and a pooled direct chunk paused mid-way: held while paused (`refCnt` 1 after the decoder has returned), body correct after resuming, released at the end; - a held direct chunk released on removal, on close and on failure; - output the inflater still holds without input: raw deflate bodies of 64 KiB plus a few bytes, of which 13 sizes pause in exactly that state, checked through package-private test accessors; - a drained or discarded channel: the body is not inflated, its end is handed on. - **`Http1ContentDecompressorTest` (#2357's, 34 tests):** its comparisons with Netty now expect more parts where Netty hands on a part larger than 64 KiB, which happens in 3 of its scenarios; bodies, errors, the end of the response and releases are compared as before, and every part must be at most 64 KiB. New: a failed decoder result on the last chunk; br, zstd and snappy decoded like Netty's decoders, with every split of #2357's helper, from heap and direct buffers; the size limit for those codings; merged parts sized for their content; read requests with auto-read off compared with Netty's for those codings too; and a class loader that hides Netty's `Decompressor` API, under which gzip and deflate decode and a zstd response fails with a `LinkageError`. The test that mixes this handler's messages with Netty's in one read now uses a transfer coding, since snappy is no longer left to Netty. - **`Http1ContentDecompressorForwardThresholdTest` (1 test):** in a fresh JVM with `-Dio.netty.compression.defaultMaxForwardBytes=0`, which hands on the output of every inflate step, a 200,000-byte gzip body is decoded at once and with a suspension after each part. The parent waits at most 60 seconds and kills the child. - **`Http1ContentDecompressorLargeOutputTest` (1 test):** in a fresh JVM with `-Dio.netty.compression.defaultMaxForwardBytes=262144`, checks that zstd output really exceeds 64 KiB, that parts stay within 64 KiB, one per resume, and that a tracking allocator sees every buffer released, also when the handler is removed while it holds the rest of a split output. Broken variants, each run against these test classes: - [x] Without the suspension check: 16 failures and 2 errors, including both bound tests, which report the 1,955,380 bytes in 30 parts from above. - [x] Read requests passed straight through: 15 failures and 2 errors, including the chunked-body test and both replay tests. - [x] Held input not handed on when the connection closes: 2 failures, the close-delimited body test and the `channelInactive` ordering test. - [x] Parts not capped at 64 KiB: 3 failures, the comparisons with Netty where a large chunk inflates at once. - [x] Each inflate call capped by the forwarding threshold instead of the part size: the zero-threshold test hits its 60-second limit, as the inflater is asked for 0 bytes forever. - [x] Held input kept as a slice without retaining it: 18 failures and 3 errors, including both direct-buffer tests. - [x] Output still held by the inflater not restored on resume: 1 failure, the held-output test, which loses the last byte of a 65,537-byte body. - [x] A drained body inflated anyway: 3 failures, the `ResponseFilter` replay test and both unit variants. - [x] A failed decoder result dropped: 1 failure, the last-chunk test. Runs: - [x] Each commit compiles with its tests. - [x] `./mvnw -B -ntp -Dgpg.skip=true clean verify` on JDK 11: 1,856 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used. - [x] The test classes above, `ResponseBodyControlTest` and `Http2ResponseBodyControlTest`, compiled on JDK 11 and run on JDK 17, 21 and 25 (Maven Surefire's `-Djvm`): 83 tests, 0 failures on each. The fresh-JVM test's child runs on the same JDK. - [x] All open AHC changes merged together (this branch, `ResponseBodyControl.execute`, raw `Cookie` headers on retries, suspend/resume order): they merge without conflicts, in every pair, and `./mvnw -B -ntp -Dgpg.skip=true clean install` on JDK 11 passes with 1,903 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. - [x] Play WS against that merged build, Scala 2.13 and 3.3, JDK 17: all tests pass. Its gzip streaming test demands one part early from another thread and finds 0 bytes delivered beyond demand, against 1,875,916 bytes with AHC 3.0.14. The installed AHC jar is byte-identical to the one built, and Play WS's shaded jar contains `PullDecompression`. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Aayush Atharva <aayush@shieldblaze.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.
Summary
IOExceptionFilter, detach the old channel from the future before completing the old response's body control.main: on HTTP/2, the handler that has just been told to retry receives frames of the abandoned response; on HTTP/1.1 with auto-read off, the old connection is never drained or closed.Problem
NettyRequestSender.replayRequestcompleted the old response'sResponseBodyControlfirst, and only then detached the old channel from the future.onRetrygot parts of the abandoned response before the replayed response's status.replayRequestinstalls a drain callback that waits for the end of the old response, but after the replay nothing read the old channel. The drain never ran, and the connection was never drained or closed.Change
With auto-read on, HTTP/1.1 on
maindelivers nothing of the old response afteronRetry, because NIO reads are asynchronous; the new order also holds when the old response holds back input that is delivered on the spot, which #2362 introduces.AI disclosure
Claude Code on behalf of Matthias Kurz. The commit includes
Co-Authored-ByperAGENTS.md.Test plan
Http2ResponseBodyControlTest.replayDoesNotDeliverFramesQueuedForTheReplacedStream: queues DATA frames in a suspended stream channel, injects an I/O error, and checks that afteronRetrythe handler receives the replayed body, not the queued frames. Fails without the change.ResponseBodyControlTest.ioExceptionReplayDrainsTheOldConnectionWithAutoReadDisabled: with auto-read off, an I/O error replays a suspended response; the old connection must be drained and closed once its last chunk arrives. Fails without the change. Over HTTPS the bug does not show, sinceSslHandlerissues reads of its own, so the test uses plain HTTP.-Djvm): 20 tests each, 0 failures../mvnw -B -ntp -Dgpg.skip=true clean verifyon JDK 11: 1,821 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used.