Add response body flow control - #2318
Conversation
| // Refill shared credit on receipt so a suspended stream cannot starve siblings. Per-stream windows retain | ||
| // application backpressure, at the deliberate cost that aggregate queued data can scale with the number of | ||
| // suspended streams. ResponseBodyControl documents the relevant configuration bounds. | ||
| connection.local().flowController(new DefaultHttp2LocalFlowController( |
There was a problem hiding this comment.
This turns the connection level bound off for every H2 connection, not only the ones that use the new control. With the shipped defaults (http2InitialWindowSize 16 MiB, http2MaxConcurrentStreams -1) nothing aggregate is left, so the ceiling becomes the per stream window times whatever concurrency the server picks. Before this a slow consumer was capped at 64 KiB per connection.
Can we auto refill only while a control on that connection is actually suspended, or keep an aggregate byte budget? I would rather not change the default for users who never touch ResponseBodyControl. The README points at http2MaxConcurrentStreams as the mitigation, but that is off by default too.
There was a problem hiding this comment.
Agreed. I chose suspension-scoped connection auto-refill and implemented it in 7bf9011.
Connections with no suspended response now retain Netty's normal 65,535-byte shared connection-window bound. When the first response on a connection is suspended, AHC returns connection credit that had already accumulated and continues returning connection-level credit while leaving per-stream credit consumption-driven. Multiple suspensions are counted, and normal connection accounting resumes after the last one ends. Credit already returned and data already queued cannot be revoked, so the remaining aggregate-buffering trade-off exists only during an active suspension and is documented in the API, README, and PR text.
The implementation also tracks connection credit returned early so application consumption after resume cannot return it twice. Tests cover the unchanged no-suspension path, DATA received before the suspension callback, overlapping suspensions, no double credit after resume, and the sibling-stream integration case.
This required adapting Netty's default local flow controller because its connection auto-refill state is private and fixed at construction; composition or subclassing cannot switch it for the lifetime of a suspension. That is a real maintenance cost, so 08be4ab5f records that the package-private implementation is forked from Netty 4.2.17.Final and that every Netty upgrade must compare it with upstream for correctness and security fixes. A runtime-switchable Netty API is the exit path that would let AHC delete the adaptation. I did not add a hard aggregate byte budget in this PR, and I have not claimed that an upstream Netty issue already exists.
| } | ||
|
|
||
| Channel channel = ctx.channel(); | ||
| NettyResponseBodyControl.discardForChannelClose(channel); |
There was a problem hiding this comment.
This runs before the two branches below that keep the exchange alive on purpose, the IOException filter one and recoverOnReadOrWriteException. In both we do not close the channel, but the control is already discarded, so the handler is left holding one where suspend(), resume() and cancel() are silent no ops. detach(false) also leaves autoRead off on a channel that survives. Should we move it down to the paths that really tear the channel down?
There was a problem hiding this comment.
Agreed. Fixed in 631bdb2.
The control is no longer discarded at the start of exceptionCaught. It is discarded only on paths that actually close the channel. Replay now completes the old control before the old response is drained, restoring reads, while a genuinely reused exchange receives a replacement control. The ownership also moved from the channel to NettyResponseFuture, so it follows the exchange and stale controls cannot affect a later exchange on the same channel.
The regression test suspends the first response, triggers an IOExceptionFilter replay, verifies that the old channel has autoRead restored for draining, verifies that a replacement control is delivered, and confirms that calls through the stale control do not affect the replayed response.
| @Override | ||
| public void channelReadComplete(ChannelHandlerContext ctx) { | ||
| readIfNeeded(ctx); | ||
| if (!NettyResponseBodyControl.isSuspended(ctx.channel())) { |
There was a problem hiding this comment.
Nit: this does an attribute lookup on every read complete for every channel, including each H2 stream child, and in the normal case readIfNeeded then no ops anyway. suspend0() always sets autoRead to false, so checking isAutoRead() first is equivalent and keeps the lookup off the hot path.
There was a problem hiding this comment.
Agreed. Fixed in 631bdb2.
channelReadComplete now checks channel.config().isAutoRead() first and returns immediately in the normal case. It only looks up the exchange/control when auto-read is already disabled, so the extra lookup is off the hot path for ordinary HTTP/1.1 channels and HTTP/2 stream children.
| if (!abort) { | ||
| NettyResponseBodyControl control = NettyResponseBodyControl.create( | ||
| channel, future::touch, () -> finishUpdate(future, channel, false)); | ||
| abort = handler.onResponseBodyStart(control) == State.ABORT; |
There was a problem hiding this comment.
When the HEADERS frame ends the stream we still create a control and call this, and finishUpdate runs a few lines down. A handler that suspends here gets no data and no error, and the resume() its consumer sends later hits !active and returns silently. Both new tests suspend unconditionally in onResponseBodyStart, so the path is exercised but the ignored suspend is never asserted. Either skip the callback when there is no body, or state on ResponseBodyControl that suspending in that case does nothing.
There was a problem hiding this comment.
I kept the callback for bodyless responses so a handler receives one consistent response-body lifecycle callback after final headers, and documented the exact limitation in dfd26a6: if the final headers also end the response, suspend() cannot defer completion. The control becomes inactive when the callback returns and later calls are no-ops.
There are now explicit HTTP/1.1 and HTTP/2 tests that suspend in this callback, verify that completion is not deferred, and verify that later control calls have no effect.
| channel, future::touch, () -> finishUpdate(future, channel, false)); | ||
| abort = handler.onResponseBodyStart(control) == State.ABORT; | ||
| if (abort) { | ||
| NettyResponseBodyControl.complete(channel); |
There was a problem hiding this comment.
finishUpdate calls complete() as its first statement, and if cancel() ran inline we already detached, so this never does anything. Same for the copy in HttpHandler.abortAfterStartingResponseBody. I would drop both and keep finishUpdate as the single completion point.
There was a problem hiding this comment.
Agreed. Fixed in 31434bb: both callback-abort copies of complete() are gone, and finishUpdate is the single control-completion point.
| if (!channel.eventLoop().inEventLoop()) { | ||
| throw new IllegalStateException("A response body control must be initialized on its channel event loop"); | ||
| } | ||
| if (get(channel) != null) { |
There was a problem hiding this comment.
Keying this on the channel is fragile. HttpHandler.handleRead returns early when applyIoExceptionFiltersAndReplayRequest replays and nothing detaches the control, so the channel enters drainChannelAndOffer still suspended with autoRead off. LastHttpContent never arrives, the drain never finishes, and the channel, its pool slot and the future are gone for good. If such a channel is reused later this throw fails a request that has nothing wrong with it. Keying the control to the exchange and replacing instead of throwing would remove both.
There was a problem hiding this comment.
Agreed. Fixed in 631bdb2.
The current control is now held atomically by NettyResponseFuture, not in a channel attribute. Creating a control replaces and deactivates the previous one instead of throwing, and stale controls become harmless no-ops. Replay explicitly completes the old control before draining its channel, restoring reads, and the next response installs a new control for the same exchange.
The replay regression test covers the suspended-channel drain, replacement control, and stale suspend()/cancel() calls without letting those calls affect the replayed response.
| if (channel.eventLoop().inEventLoop()) { | ||
| task.run(); | ||
| } else { | ||
| channel.eventLoop().execute(task); |
There was a problem hiding this comment.
ResponseBodyControl says calls made after completion have no effect, but this throws RejectedExecutionException at the caller once the event loop group is shutting down, which is exactly when a consumer calls cancel(). Catch it and treat it as the documented no op.
There was a problem hiding this comment.
Agreed. Fixed in 631bdb2.
NettyResponseBodyControl.execute now catches RejectedExecutionException; once the channel event loop is shutting down there is no remaining transport for the control to affect, so this follows the documented post-completion no-op behavior. A regression test closes the client and then calls suspend(), resume(), and cancel() through the retained control, verifying that none throws.
| return; | ||
| } | ||
|
|
||
| if (NettyResponseBodyControl.isSuspended(nettyResponseFuture.channel())) { |
There was a problem hiding this comment.
With the request timeout disabled this re-arms forever and never reaches clean(), so a handler that suspends and then loses its consumer pins the future, the channel and the AsyncHandler for the life of the process. The description mentions the risk but there is no bound in the code. Can we cap the total suspended time, or at least log once when it runs long?
There was a problem hiding this comment.
I took the logging option and implemented it in ea01b27.
When the read-timeout interval expires while the response is still suspended and the request timeout is disabled, AHC now logs one WARN for that exchange explaining that transport resources remain retained until the application resumes or cancels it. It logs only once even though the read timeout continues to re-arm. I did not impose a new suspension deadline because that would turn an intentionally disabled request timeout into a different cancellation policy.
The test verifies the warning level, the request-timeout-disabled wording, and one-shot behavior.
| } | ||
|
|
||
| public void cancel() { | ||
| public synchronized void cancel() { |
There was a problem hiding this comment.
cancelTimeouts() is called from finishUpdate on the event loop for every request, so this monitor now sits on the I/O path. The timer thread holds the same monitor inside startReadTimeout across nettyTimer.newTimeout(), which blocks on first use and contends on the wheel's queue. On an H2 connection that parks the loop for every other stream on it. Re-checking cancelled after scheduling, or a CAS on the readTimeout field, closes the same race without a shared lock.
There was a problem hiding this comment.
Agreed. After rebasing onto current main, the synchronized implementation is gone. The upstream timeout changes now use an AtomicBoolean for one-shot cancellation, schedule without a shared monitor, and call cancelIfRaced after recording the scheduled handle. That closes the arm/cancel race without putting a monitor on the event-loop completion path. The suspension warning in ea01b27 builds on that lock-free implementation.
| private int serverPort; | ||
|
|
||
| @BeforeEach | ||
| public void prepareServer() throws Exception { |
There was a problem hiding this comment.
Nit: this is the same bootstrap as Http2StreamingBodyFlowControlTest and five other H2 tests. Worth pulling into a helper next to TestUtils at some point so the next pkitesting or ALPN change is one edit instead of eight.
There was a problem hiding this comment.
Agreed that the repeated HTTP/2 bootstrap is worth consolidating. I kept that test-infrastructure refactor out of this PR because doing it properly would touch the existing HTTP/2 test classes as well and would add unrelated churn around the flow-control change. I will leave this as a focused follow-up rather than extracting a helper used only by the new test here.
Expose a thread-safe response body control after final response headers so handlers can suspend, resume, or cancel transport reads without coupling AHC to a streaming API. Integrate the control with HTTP/1.1 and HTTP/2, pause network read timeouts while reads are intentionally suspended, and keep request timeouts active. Make completion, cancellation, callback aborts, and channel teardown restore transport state exactly once. Cover backpressure, cancellation, timeout, callback failures, and connection reuse for both protocols. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Return connection-level receive credit as DATA frames arrive while preserving per-stream flow control for application backpressure. This prevents one suspended response from starving siblings on the same multiplexed connection. Document that aggregate queued data can scale with the number of suspended streams and point users to the initial-window and concurrent-stream controls. Preserve Netty's client shutdown behavior in the custom frame-codec builder. Cover sibling progress, repeated cancellation, and the retained per-stream window bound. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Store each response body control on its response future instead of its transport channel. Replays can then replace the control without leaving a suspended channel behind, and calls on a stale control become harmless. Keep recoverable exception paths alive until their replay decision is made, avoid the control lookup on the normal auto-read path, and tolerate event loop shutdown racing with late control calls. Cover replay replacement, old-channel draining, stale controls, and calls made after client shutdown. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Record when HTTP/1.1 has received the terminal content before invoking trailer or body callbacks. Cancellation from either terminal callback can then finish normally and reuse a keep-alive connection instead of closing it as unread. Check for inline cancellation after trailers so no later terminal body callback is delivered. Remove duplicate control completion from callback-abort paths and leave finishUpdate as the single completion owner. Cover cancellation from terminal body and trailer callbacks, single handler completion, skipped callbacks, and HTTP/1.1 connection reuse. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Document that onResponseBodyStart is invoked even when final headers end the response, but suspension cannot defer completion in that case. The control is inactive once the callback returns and later calls are harmless. Cover that behavior explicitly for HTTP/1.1 and HTTP/2, including completion without resume, late control calls, and connection reuse. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Treat HTTP/1.1 informational responses as interim rather than delivering them to the response handler or creating a response body control. Ignore Netty's synthetic LastHttpContent for each interim response while preserving the special terminator used to release a deferred 100-continue request body. Cover a 103 Early Hints followed by a final response, callback counts, body delivery, completion, and connection reuse. Retain the existing deferred 100-continue behavior. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Log once when a response remains suspended for a read-timeout interval while its request timeout is disabled. Such an exchange otherwise has no configured deadline and retains its handler and transport resources until application code resumes or cancels it. Keep the timeout path lock-free using the race-safe arming logic already on main, and cover warning severity, wording, and one-shot behavior. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Preserve the normal connection-level receive-window bound until a handler actually suspends a response. While any response on the connection is suspended, return shared credit without returning its per-stream credit so unrelated streams can continue. Track credit returned early until application consumption catches up. This avoids returning connection credit twice when suspension ends and normal accounting resumes. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Use the existing last-content callback, tied to the response future, to consume Netty's synthetic terminator after an HTTP/1.1 interim response. This prevents a channel marker from surviving into a later exchange. Explain how the unchanged channel attribute identifies an unsolicited 100 response after the continue interceptor runs. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Record that the package-private controller is adapted from Netty 4.2.17.Final and must be compared with upstream when Netty is upgraded for correctness and security fixes. Identify a runtime auto-refill API in Netty as the exit path that would let AHC remove the maintained adaptation. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
00ada81 to
08be4ab
Compare
Summary
ResponseBodyControlcallback after final response headers.Motivation
AHC 3 removed
StreamedAsyncHandlerand its Reactive Streams integration in pull request #1843. That removal avoids coupling AHC to a particular streaming library, butAsyncHandlerby itself has no way to stop transport reads while a downstream consumer has no demand.Play WS is the driving consumer for this change. Play WS needs transport backpressure to preserve its existing Pekko Streams and Reactive Streams response APIs while upgrading to AHC 3. A working adapter on a currently local Play WS development branch turns this control into a single-subscriber Reactive Streams publisher and has been tested against this AHC branch.
This pull request adds only the transport primitive. Streaming-library policy and dependencies remain in Play WS, so AHC does not regain a dependency on Reactive Streams or JDK Flow.
Semantics
AsyncHandler.onResponseBodyStartruns after final headers and before body parts, including for a response with no body.suspend()stops requesting new transport data, although body parts already read may still be delivered.resume()permits transport reads again.State.ABORTis the synchronous callback-time way to stop processing. A handler can retain the control and callcancel()when an asynchronous decision is made after the callback returns.HTTP/2 flow control
Connections with no actively suspended response retain Netty's normal connection-level receive-window accounting. The default 65,535-byte connection window therefore continues to cap unconsumed flow-controlled DATA across all streams for users that never call
suspend().When the first response on a connection is suspended, AHC returns connection-level credit that has already accumulated and continues returning that shared credit as DATA arrives. This prevents the suspended stream from exhausting the connection window and starving sibling streams. Per-stream credit remains consumption-driven at all times, so each suspended stream is still bounded by its own receive window. Multiple simultaneous suspensions are counted, and normal connection accounting resumes after the last one ends. Credit already returned and data already queued cannot be revoked, and the controller tracks credit returned early so later application consumption does not return it twice.
During an active suspension, aggregate buffering can still scale with the number of concurrent streams; the relevant controls are
http2InitialWindowSize,http2MaxConcurrentStreams, and the connection limits. The defaults do not impose a hard client-side aggregate bound during that interval: the initial per-stream window is 16 MiB andhttp2MaxConcurrentStreams = -1leaves concurrency server-controlled. A rough upper-bound estimate is connection count times effective concurrent streams times the initial window, excluding network and decoder overhead. Applications requiring a finite policy must configure these values together. A hard aggregate byte budget is a separate design and is outside this pull request.Netty's connection auto-refill state is private and fixed when
DefaultHttp2LocalFlowControlleris constructed, so it cannot be enabled only for the lifetime of a suspension through composition or subclassing. The package-privateSuspensionAwareHttp2LocalFlowControllertherefore adaptsDefaultHttp2LocalFlowControllerfrom Netty 4.2.17.Final, preserving its normal behavior while making connection refill suspension-scoped.This adaptation has a maintenance cost: AHC owns the copied flow-control logic and every Netty upgrade must compare it with the corresponding upstream implementation for correctness and security fixes. The exact source version and that obligation are recorded in the class Javadoc. An upstream Netty API that permits connection auto-refill to be changed at runtime would provide the exit path and allow AHC to remove the adaptation; no such API exists in Netty 4.2.17.Final.
The auto-refill mode requires a custom
Http2Connection. Netty's builder treatsserver()andconnection()as mutually exclusive, soClientHttp2FrameCodecBuildersupplies the connection through the protected builder API and overridesisServer()to retain client mode.The explicit
gracefulShutdownTimeoutMillis(0)is not a new shutdown policy.Http2FrameCodecBuilder.forClient()selects zero through its package-private client constructor; the subclass must use the protected no-argument constructor, so it sets zero explicitly to preserve the existing client-factory behavior.Scope and commit structure
The API/lifecycle work and the HTTP/2 independence work are kept as separate logical commits, but they belong in one pull request. Without the HTTP/2 work, a suspended response can consume the shared connection window and block unrelated sibling streams, so the public control would not have correct multiplexed behavior. Follow-up review fixes are also split into focused commits covering exchange ownership, terminal HTTP/1.1 cancellation, bodyless responses, interim responses, indefinite-suspension diagnostics, and suspension-scoped HTTP/2 refill.
History checked
AsyncHandlerbackpressure, and pull request #963 addressed it by adding Reactive Streams support.StreamedAsyncHandlerfor AHC 3.Compatibility
AsyncHandler.onResponseBodyStartis a new Javadefaultmethod, so existing handler implementations remain source- and binary-compatible and retain their previous behavior unless they override it.ResponseBodyControlis a new public interface.NettyResponseBodyControlis public only to support AHC's cross-package transport integration and is marked@ApiStatus.Internal; consumers should depend onResponseBodyControlinstead.onStatusReceivedoronHeadersReceived; this matches the existing HTTP/2 behavior and prevents a 103 Early Hints response from completing the exchange before the final response. Existing 100 Continue handling and 101 protocol switching are preserved.LastHttpContenthas already been received; it previously closed that fully read connection.AI disclosure
OpenAI Codex on behalf of Matthias Kurz. The commits include
Co-Authored-By: OpenAI Codex <codex@openai.com>perAGENTS.md.Test plan
ResponseBodyControlTest,Http2ResponseBodyControlTest,TimeoutTimerTaskTest,SuspensionAwareHttp2LocalFlowControllerTest, andContinue100InterceptorTestsuites pass 29 tests: 12 HTTP/1.1 control tests, 6 HTTP/2 integration tests, 4 timeout-task tests, 5 flow-controller accounting tests, and 2 HTTP/1.1 Continue tests.JAVA_HOME=<jdk-11> ./mvnw clean verify: BUILD SUCCESS for the full reactor, including tests, Javadocs, coverage, and Revapi.