Repository navigation
Conversation
Motivation: HTTP/1.1 automatic decompression ran Netty's HttpContentDecompressor, which inflates every chunk completely as soon as it is read. Suspending a response through ResponseBodyControl stops further socket reads, but not that. One 64 KiB read of a highly compressible gzip body (about 1032:1) still became some 67 MB of body parts for a handler that had asked for none; only maxDecompressedResponseSize (256 MiB) bounded it. AHC 2 behaved the same way, so this is hardening, not a regression fix. Modification: Http1ContentDecompressor is now a ChannelDuplexHandler built on Netty's pull-based Decompressor (Netty 4.2.17+; Netty still marks this API unstable). It keeps the codings (gzip, including bodies of several gzip members, deflate, br, snappy, zstd), the header rewrites and the maxDecompressedResponseSize accounting. It hands the body on in parts of at most 64 KiB, splitting larger decompressor output, and checks before each part whether the response is suspended. If it is, the compressed input stays in the handler, and so does every message behind it. The read request that resume() issues continues decompression before it reaches the socket. Without suspension, each part goes straight through as before. The pipeline is torn down with its channel, so if the connection closes while input is held back, the rest is inflated and delivered before channelInactive. Once the exchange has ended (cancel, ABORT, timeout), held input is dropped instead of inflated. A framing failure on the final chunk now reaches the handler; Netty's decoder used to replace it with success. Revapi reports the changed superclass of this public handler class and the members inherited from Netty's decoder; they are justified in the pom. HTTP/2 is unchanged. After END_STREAM, Netty's stream channel hands every queued frame over on the next read and then closes. A handler in the stream pipeline therefore cannot hold compressed input back. Result: A response whose handler suspends it after each body part receives one part of at most 64 KiB per resume, and decompression runs ahead of the delivered parts by at most one decompressor output buffer. For 64 MiB of gzipped zeros, main delivered 1.96 MB in 30 more parts past the suspending part, then up to 33.7 MB per resume; now nothing past it, and 65,536 bytes per resume. Http1DecompressionSuspensionTest covers auto-read on and off, chunked bodies with trailers, close-delimited bodies, cancel, ABORT, bodiless responses and connection reuse; its two bound tests fail on main. Http1ContentDecompressorTest covers the handler on its own, including gzip bodies of several members, corrupt and trailing data, and every coding across repeated suspension. Http1ContentDecompressorLargeOutputTest covers decompressor output larger than a part, in a JVM with Netty's io.netty.compression.defaultMaxForwardBytes raised. Loopback throughput without suspension: unchanged for gzipped zeros, about 5% more elapsed time for gzipped text. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Heads-up: #2357, opened before this PR, which I missed, rewrites the same If you'd like #2357 first, I'd rebuild the demand bounding on top of its inflate loop instead of Netty's pull |
|
Related to "Why HTTP/2 is unchanged" above: netty/netty#17774 makes |
| } | ||
|
|
||
| @Override | ||
| public void read(ChannelHandlerContext ctx) { |
There was a problem hiding this comment.
This drain also runs from replayRequest. It calls NettyResponseBodyControl.complete(future) before drainChannelAndOffer swaps the attribute, and setAutoRead(true) issues a channel.read() right there. So whatever is held back gets inflated under the old future and the handler that just got onRetry() receives the old body. I reproduced it with an injected IOException and a replaying IOExceptionFilter, 31 parts of the first response arrived before the retried status. With AUTO_READ off nothing issues that read at all, so the old channel is never drained and never closed. We should install the drain callback before complete(future) and read once when autoRead is off. Not reachable on main since nothing was held back there.
There was a problem hiding this comment.
Confirmed and fixed in 5604fea. replayRequest now installs the drain callback (HTTP/1.1) or the discard marker (HTTP/2) before complete(future). With auto-read off, it issues one read so the drain starts.
The new tests inject the IOException the way you did, after the body callback and the decompressor's pass have returned. They run with auto-read on and off, check that the retry's onStatusReceived comes right after onRetry, and check that the old connection is drained and closed.
The same ordering also affected HTTP/2, on main too. Completing the control turns auto-read back on, and the stream channel then hands its queued DATA frames to the retried handler right away. Http2ResponseBodyControlTest.replayDoesNotDeliverFramesQueuedForTheReplacedStream fails on main: the retried handler first gets the old stream's first frame. Before it injects the failure, the test waits until the client has acknowledged a PING that the server sent after the DATA frames, so the frames are known to wait in the stream channel.
| closeDecompressor(); | ||
| continue; | ||
| } | ||
| if (!inputClosed && isSuspended(channel)) { |
There was a problem hiding this comment.
This is the bomb case the PR is about, just with a close after it. A suspended handler gets the whole remainder in one go with no resume, bounded only by maxDecompressedResponseSize. I get that the pipeline is gone right after, so no code fix here, but the javadoc on suspend() and the behavior changes section should say that close delimited bodies are not bounded. Worth a follow-up next to the HTTP/2 one.
There was a problem hiding this comment.
Agreed, no code change. Documented in b5db977, in suspend() and in the class Javadoc, and added to the description. If the connection closes while input is held back, the rest is inflated and delivered at once, even while suspended, and only maxDecompressedResponseSize bounds it. That applies to any close that finds input held back, not only to close-delimited bodies. I'll take it up as a follow-up alongside the HTTP/2 one, although the two may need different fixes.
| if (part != null) { | ||
| ctx.fireChannelRead(new DefaultHttpContent(part)); | ||
| } | ||
| } else if (lastContent != null) { |
There was a problem hiding this comment.
Same for the last content, it goes through while suspended, so a handler that suspends on the last data part still sees the trailers and onCompleted(). Fine if intended, but then say so next to the no further part until resume statement, or hold it until the next read.
There was a problem hiding this comment.
Intended, and now documented next to that statement (b5db977). Only parts with content wait for a resume. Once the body has been inflated in full, the empty last part, trailers and onCompleted() follow even while suspended, as they do for an unencoded body.
I chose this over holding the end back because a Reactive Streams consumer, such as Play WS's publisher, resumes only while it has demand. It still has to signal completion to a subscriber that has received exactly what it requested. If the end waited for another resume, such a stream would never complete. A test pins this behavior, also for a compressed response without body bytes.
| } else { | ||
| if (!merged) { | ||
| ByteBuf first = part; | ||
| part = alloc.heapBuffer(MAX_PART_SIZE); |
There was a problem hiding this comment.
This always allocates MAX_PART_SIZE even when the merged part ends up a few hundred bytes, and a consumer that keeps parts around holds 64 KiB for each of them. Size it to what is actually being merged, the buffer grows if it has to.
There was a problem hiding this comment.
Fixed in 9e825e6. The merged part is allocated for its first two buffers and grows as needed, up to 64 KiB. Ten 100-byte gzip members now give one part of 1,000 bytes with a capacity of 1,024 instead of 65,536.
| import io.netty.handler.codec.compression.BrotliDecompressor; | ||
| import io.netty.handler.codec.compression.DecompressionException; | ||
| import io.netty.handler.codec.http.HttpContentDecompressor; | ||
| import io.netty.handler.codec.compression.Decompressor; |
There was a problem hiding this comment.
This class only exists since 4.2.17, so anyone managing netty.version below that gets a NoClassDefFoundError on every HTTP/1.1 channel init, not just on compressed responses. Needs at least a release note.
There was a problem hiding this comment.
Agreed. The Compatibility section of the description now has it as a release note. AHC now needs Netty 4.2.17 or later. With an older netty.version, the NoClassDefFoundError comes when an HTTP/1.1 pipeline is set up with automatic decompression on, the default, so it also hits requests whose responses are not compressed. The class Javadoc says so too (b5db977).
| && decompressor.status() == Decompressor.Status.NEED_INPUT) { | ||
| decompressor.addInput(input.retain()); | ||
| } | ||
| if (content instanceof LastHttpContent) { |
There was a problem hiding this comment.
We never call endOfInput(), so a body cut off mid member still completes as a success and a missing CRC32 is never checked. Same as main, but with this API it is a two liner now, call it when any body byte was fed and keep empty bodies as they are. Follow-up is fine.
There was a problem hiding this comment.
Agreed, I'll send it as a follow-up. Failing a body that ends in the middle of a member changes behavior that main has, so it seems worth its own PR. It also has to avoid calling endOfInput() while held input or output is still pending, and keep empty bodies and concatenated members working.
| && NettyResponseBodyControl.isSuspended((NettyResponseFuture<?>) attribute, channel); | ||
| } | ||
|
|
||
| private static boolean isAbandoned(Channel channel) { |
There was a problem hiding this comment.
A drain callback is neither suspended nor abandoned, so a redirect or 401 with a compressed body still inflates all of it up to the limit, and every part is copied and released right away. Same as main, but treating OnLastHttpContentCallback like DISCARD here would make the drain almost free.
There was a problem hiding this comment.
Done in 7a00b8a. A drain callback is now treated like DISCARD, so the rest of the body is dropped instead of inflated, and the last chunk still reaches the callback.
One detail: redirects and 401/407 challenges with a compressed body rarely drain here. The decompressor rewrites Content-Length to chunked, as Netty's does, and those interceptors close the connection for a chunked response instead of draining it. The drain that did inflate is the one after a replay from a ResponseFilter. The new test replays on a compressed response far beyond the limit, and waits until the pool has accepted the drained connection. Before the fix, the failed drain closed the connection and could fail the replayed request, which shares the future.
| * bounds how far the body may inflate in total. | ||
| * <p> | ||
| * Netty's {@code HttpContentDecompressor} inflates each compressed chunk completely as soon as it is read. | ||
| * Suspending a response through {@link org.asynchttpclient.ResponseBodyControl} stops further socket reads but |
There was a problem hiding this comment.
Import ResponseBodyControl instead, AGENTS.md asks for no fully qualified names.
Motivation: replayRequest completed the old response's body control before it detached the old channel from the future. Completing the control restores auto-read, and the read that issues delivers what the old response still holds back on the spot: input held by a suspended HTTP/1.1 decompressor, or DATA frames queued in a suspended HTTP/2 stream channel. 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. With auto-read off, nothing read the old HTTP/1.1 channel at all, so it was never drained and never closed. The HTTP/1.1 case comes with the demand-bounded decompressor; the HTTP/2 case also happens on main. Modification: Install the HTTP/1.1 drain callback, or mark the HTTP/2 stream channel as discarded, 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. The HTTP/1.1 tests, with auto-read on and off, inject the failure after the body callback and the decompressor have returned. The HTTP/2 test queues DATA frames in the suspended stream channel; it fails on main. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Motivation: While a channel is drained after a redirect, an authentication challenge or a replay, AsyncHttpClientHandler discards every body part until the last one. The decompressor still inflated a compressed body in full and copied each part before it was dropped. A body beyond maxDecompressedResponseSize failed the drain, which closed the connection and could fail the request that had moved on with the same future. main behaves the same way. Modification: Treat a channel whose attribute is an OnLastHttpContentCallback like a discarded one: drop the remaining input instead of inflating it. The terminating chunk is still handed on, so the callback still runs. Result: When a response filter replays the request on a compressed response that inflates far beyond the limit, the replayed request succeeds and the drained connection returns to the pool. Redirects and 401 or 407 challenges with a compressed body rarely get here: the decompressor rewrites Content-Length to chunked, as Netty's does, so they close the connection instead of draining it. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Motivation: A part merged from several small decompressor buffers was always allocated with a capacity of 64 KiB, even when it held a few hundred bytes, and a consumer that keeps parts holds that capacity for each. Modification: Allocate the merged part for the first two buffers and let it grow, up to 64 KiB, as more are copied in. Result: Ten gzip members of 100 bytes become one part of 1,000 bytes with a capacity of 1,024 bytes instead of 65,536. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Motivation: Neither the decompressor nor ResponseBodyControl.suspend() said which parts of a compressed HTTP/1.1 body a suspension does not hold back, or which Netty version the decompressor needs. Modification: Document in suspend() and in the decompressor that: - only parts with content wait for a resume. Once the body has been inflated in full, its last part, trailers and completion arrive even while suspended, as for an unencoded body; - if the connection closes while input is held back, the rest is inflated and delivered at once, bounded only by maxDecompressedResponseSize. Note that the decompressor needs Netty 4.2.17 or later, and import ResponseBodyControl instead of naming it in full. Result: A test pins that the end of a fully inflated body, and of a body without bytes, does not wait for a resume. Holding it back would leave a consumer whose demand ended with the last part without completion. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. I've pushed four commits on top, one per fix, and updated the description: the Compatibility section now states that Netty 4.2.17 is required, and Behavior changes covers the replay fix, which also changes HTTP/2. The |
Summary
ResponseBodyControl, stop decompressing its body. Decompressed body parts are at most 64 KiB, so a handler that suspends after each part receives one part perresume().Http1ContentDecompressorbecomes aChannelDuplexHandleron Netty's pull-basedDecompressor, instead of extending Netty'sHttpContentDecompressor. Codings, multi-member gzip, header rewrites and themaxDecompressedResponseSizelimit are kept.IOExceptionFilteror aResponseFilter) now detaches the old channel before it completes the old response's body control, so the retried handler gets nothing of the old response. This also fixes HTTP/2 onmain.Problem
Suspending a response stops further socket reads, but Netty's
HttpContentDecompressorinflates 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) became some 67 MB of body parts for a handler that had suspended the response. Only
maxDecompressedResponseSize(256 MiB by default) bounded it.With 64 MiB of gzipped zeros and a handler that suspends inside each body callback:
maindelivered 1,955,380 bytes in 30 more parts past the suspending part;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
Http1ContentDecompressoruses Netty's pull-basedDecompressor(Netty 4.2.17+, AHC pins 4.2.18):100 Continuepass-through, and themaxDecompressedResponseSizeaccounting and message. The limit counts every gzip member.io.netty.compression.defaultMaxForwardBytesis raised, is split into retained slices. A slice shares the larger buffer until the slice and the rest have both been released, so the 64 KiB bound applies to delivered parts, not to allocations. Before each part, the handler checks whether the response is suspended.resume()issues. It continues decompression first, and only then lets the read reach the socket. This works the same with auto-read on and off.channelInactive, even to a suspended response, because Netty tears the pipeline down with the channel. Suspension no longer bounds that output; onlymaxDecompressedResponseSizedoes.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. The terminating chunk still reaches a drain callback.main. A merged part is allocated for what it holds and grows as needed.Replays
replayRequestcompleted the old response's body control before it detached the old channel from the future. Completing the control turns auto-read back on, and the read that this issues delivers whatever the old response still holds back right away:main).The channel still named the future, so the handler that had just been told to retry received parts of the abandoned response before the retry's status. With auto-read off, nothing read the old HTTP/1.1 channel, so it was never drained or closed. The old channel is now detached first: through the drain callback on HTTP/1.1, through the discard marker on HTTP/2. With auto-read off, one read starts the drain.
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 ofautoRead, 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 afterEND_STREAMand close only when drained), now proposed in netty/netty#17774, or AHC delivering the body independently of the stream channel.Behavior changes
maxDecompressedResponseSize, not by demand. This applies to any close that finds input held back, not only to bodies delimited by the close.onRetry(), the handler no longer receives parts of the old response, on HTTP/1.1 and HTTP/2.maxDecompressedResponseSizefailed the drain, closed its connection and could fail the request that followed on the same future, for example when aResponseFilterreplays 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 onmain.-Dio.netty.noJdkZlibDecoderis no longer honored on HTTP/1.1; the JDK's zlib is always used.mainon the same inputs:main, 120 ms here).Compatibility
Http1ContentDecompressoris a public class, so this is a binary-incompatible change to it: Revapi reports its changed superclass, plus the members inherited from Netty's decoder and its hooks. AHC only uses it internally (ChannelManagerinstantiates it, and its public constructor is unchanged), so nothing changes for code that uses AHC's client API. Each difference is listed as an exact Revapi entry with a justification inpom.xml, so any other change to the class is still flagged.DecompressorAPI in 4.2.17. With automatic decompression on, the default, AHC creates this handler for every HTTP/1.1 connection, so an application that pinsnetty.versionbelow 4.2.17 gets aNoClassDefFoundErrorwhen an HTTP/1.1 connection is set up, also for responses that are not compressed. Please mention this in the release notes.DecompressorAPI as unstable (@UnstableApi) while it migrates its codecs to it (netty/netty#16743). A future Netty release may require changes to this handler.Follow-ups
endOfInput()at the end of the body, so a body cut off in the middle of a gzip member, or missing its checksum, fails instead of completing. That changes behaviormainhas, so it gets its own pull request.AI disclosure
Claude Code on behalf of Matthias Kurz. The commits include
Co-Authored-ByperAGENTS.md.Test plan
New tests:
Http1DecompressionSuspensionTest(10 tests, live server):ABORT, andHEADand empty gzip responses;IOExceptionis injected after the body callback and the decompressor's pass have returned. The test checks that the retry's status followsonRetry()directly and that the old connection is drained and closed;ResponseFilterreplay on a compressed response that inflates far beyond the limit: the replayed request succeeds, and the test waits until the pool has accepted the drained connection.On
main, its two bound tests fail deterministically.Http1ContentDecompressorTest(22 tests,EmbeddedChannel):100 Continue;Http2ResponseBodyControlTest(1 new test): a replay while DATA frames wait in the suspended stream channel. Before the failure is injected, the client acknowledges a PING that the server sent after the DATA frames, so they are known to have arrived. The retried handler must get the replayed response, not the old stream's frames. It fails onmain.Http1ContentDecompressorLargeOutputTest(1 test):-Dio.netty.compression.defaultMaxForwardBytes=262144, because Netty reads that property once per JVM. The child writes to a file, the test fails if the child has not finished after 60 seconds, and the child is killed in any case. With a deliberately hanging child, the test failed after 64 seconds and left no process behind;Broken variants of the first revision, each run against its 27 tests:
channelInactiveordering test.Broken variants of the review fixes, each run against
Http1DecompressionSuspensionTestandHttp1ContentDecompressorTest(32 tests):onRetry().ResponseFilterreplay test fails, because the failed drain closes the connection instead of returning it to the pool. Whether the replayed request also fails depends on timing.main: the newHttp2ResponseBodyControlTesttest fails, because the retried handler first gets the old stream's framea.Runs:
./mvnw -B -ntp -Dgpg.skip=true clean verifyon JDK 11: 1,825 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used.Http1DecompressionLimitTest,AutomaticDecompressionTest,Http2ContentDecompressorTest,ResponseBodyControlTest, the rest ofHttp2ResponseBodyControlTest,Expect100ContinueTestandLargeResponseTest), compiled on JDK 11 and run on JDK 17, 21 and 25 (Maven Surefire's-Djvm): 73 tests, 0 failures on each. The fresh-JVM test's child runs on the same JDK.HttpContentDecompressor(whatmainuses) on 15 kinds of input, each in one buffer and in 1-byte fragments (concatenated, truncated and corrupt gzip members, trailing data after gzip, zlib and raw deflate streams): every input succeeds or fails as onmain, with the same body when it succeeds. In 5 of the 30 runs both fail, and this branch hands on fewer bytes before the failure. Run on the first revision; the review fixes don't change how a body is decoded.Cookieheaders on retries,ResponseBodyControl.execute, suspend/resume order, demand-bounded decompression): they merge without conflicts, together and in every pair, and./mvnw -B -ntp -Dgpg.skip=true clean installon JDK 11 passes with 1,861 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. Rerun with the review fixes of fix(netty): never let a queued suspend override a later resume #2361 and this pull request.