Skip to content

fix(netty): detach a replayed request's old channel before completing its body control - #2367

Merged
hyperxpro merged 1 commit into
AsyncHttpClient:mainfrom
mkurz:fix/replay-detach-old-channel
Nov 4, 2026
Merged

hyperxpro merged 1 commit into
AsyncHttpClient:mainfrom
mkurz:fix/replay-detach-old-channel

Conversation

@mkurz

@mkurz mkurz commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • When a request is replayed, for example by an IOExceptionFilter, detach the old channel from the future before completing the old response's body control.
  • Fixes two bugs on 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.
  • Split out of fix(netty): stop inflating a suspended HTTP/1.1 response #2362 at the maintainer's request, since it does not depend on either decompressor.

Problem

NettyRequestSender.replayRequest completed the old response's ResponseBodyControl first, and only then detached the old channel from the future.

  • HTTP/2: completing the control resumes a suspended stream channel, and the stream channel delivers the DATA frames queued in it on the spot. The channel still named the future, so the handler that had just received onRetry got parts of the abandoned response before the replayed response's status.
  • HTTP/1.1 with auto-read off: replayRequest installs 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

  • HTTP/2: mark the stream channel as discarded before completing the control, then close it as before.
  • HTTP/1.1: install the drain callback before completing the control, and when auto-read is off, issue one read so the drain starts.

With auto-read on, HTTP/1.1 on main delivers nothing of the old response after onRetry, 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-By per AGENTS.md.

Test plan

  • Http2ResponseBodyControlTest.replayDoesNotDeliverFramesQueuedForTheReplacedStream: queues DATA frames in a suspended stream channel, injects an I/O error, and checks that after onRetry the 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, since SslHandler issues reads of its own, so the test uses plain HTTP.
  • Broken variants: completing the control before marking the HTTP/2 stream discarded fails only the HTTP/2 test; dropping the read fails only the HTTP/1.1 test.
  • Both test classes, compiled on JDK 11 and run on JDK 17, 21 and 25 (Maven Surefire's -Djvm): 20 tests each, 0 failures.
  • ./mvnw -B -ntp -Dgpg.skip=true clean verify on JDK 11: 1,821 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used.

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
hyperxpro merged commit 24ba36a into AsyncHttpClient:main Nov 4, 2026
25 of 26 checks passed
@mkurz
mkurz deleted the fix/replay-detach-old-channel branch November 4, 2026 19:28
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>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants