Skip to content

Prepare for v3.0.0 - #1843

Merged
hyperxpro merged 20 commits into
mainfrom
develop
Jan 8, 2023
Merged

hyperxpro merged 20 commits into
mainfrom
develop

Conversation

@hyperxpro

Copy link
Copy Markdown
Member

No description provided.

@hyperxpro
hyperxpro merged commit 1b63e0b into main Jan 8, 2023
@hyperxpro
hyperxpro deleted the develop branch January 8, 2023 16:21
hyperxpro added a commit that referenced this pull request Aug 31, 2026
## Summary

- Add a thread-safe `ResponseBodyControl` callback after final response
headers.
- Support suspending, resuming, and cancelling HTTP/1.1 and HTTP/2
response bodies without coupling AHC to a streaming API.
- Pause the network read timeout while reads are intentionally suspended
while leaving the request timeout active.
- Keep suspended HTTP/2 streams independent and cover transport
backpressure, cancellation, timeouts, and connection reuse.

## Motivation

AHC 3 removed `StreamedAsyncHandler` and its Reactive Streams
integration in pull request
[#1843](#1843).
That removal avoids coupling AHC to a particular streaming library, but
`AsyncHandler` by itself has no way to stop transport reads while a
downstream consumer has no demand.

[Play WS](https://github.com/playframework/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.onResponseBodyStart` runs after final headers and before
body parts, including for a response with no body.
- Calls to the supplied control are thread-safe, idempotent, and ignored
after response completion.
- `suspend()` stops requesting new transport data, although body parts
already read may still be delivered.
- `resume()` permits transport reads again.
- Returning `State.ABORT` is the synchronous callback-time way to stop
processing. A handler can retain the control and call `cancel()` when an
asynchronous decision is made after the callback returns.
- Cancelling an HTTP/1.1 body closes its connection when bytes may
remain unread; cancellation after terminal content has been received can
reuse a keep-alive connection. Cancelling an HTTP/2 body closes only its
stream.
- Fully consumed responses retain the existing HTTP/1.1 pooling and
HTTP/2 parent-connection reuse behavior.
- Suspension pauses only the network read timeout. The request timeout
remains active; if the request timeout is disabled, an application that
never resumes or cancels can retain the exchange and its transport
resources indefinitely.

## 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 and `http2MaxConcurrentStreams = -1` leaves 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
`DefaultHttp2LocalFlowController` is constructed, so it cannot be
enabled only for the lifetime of a suspension through composition or
subclassing. The package-private
`SuspensionAwareHttp2LocalFlowController` therefore adapts
`DefaultHttp2LocalFlowController` from 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 treats `server()` and `connection()` as mutually exclusive, so
`ClientHttp2FrameCodecBuilder` supplies the connection through the
protected builder API and overrides `isServer()` 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
cleanup, terminal HTTP/1.1 cancellation, bodyless responses, interim
responses, indefinite-suspension diagnostics, and suspension-scoped
HTTP/2 refill.

## History checked

- Issue
[#544](#544)
originally identified the lack of `AsyncHandler` backpressure, and pull
request
[#963](#963)
addressed it by adding Reactive Streams support.
- Issues
[#1233](#1233)
and
[#1721](#1721)
document the interaction between downstream demand and read timeouts in
the former streamed handler.
- Pull request
[#1843](#1843)
removed `StreamedAsyncHandler` for AHC 3.
- Discussion
[#1925](#1925)
asks how to migrate streamed consumers to AHC 3; the maintainer response
declines restoring Reactive Streams because other libraries provide that
policy and maintaining it adds overhead.
- Focused GitHub issue, pull-request, discussion, and local history
searches found no existing AHC 3 proposal for a
streaming-library-neutral suspend/resume/cancel response-body control.

## Compatibility

- `AsyncHandler.onResponseBodyStart` is a new Java `default` method, so
existing handler implementations remain source- and binary-compatible
and retain their previous behavior unless they override it.
- `ResponseBodyControl` is a new public interface.
- `NettyResponseBodyControl` is public only to support AHC's
cross-package transport integration and is marked `@ApiStatus.Internal`;
consumers should depend on `ResponseBodyControl` instead.
- HTTP/1.1 informational responses from 102 through 199 are now treated
as interim and no longer reach `onStatusReceived` or
`onHeadersReceived`; 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.
- Cancelling HTTP/1.1 processing from a terminal trailer or body
callback now reuses a keep-alive connection because `LastHttpContent`
has already been received; it previously closed that fully read
connection.
- The full JDK 11 verification, including Revapi, passes.

## AI disclosure

OpenAI Codex on behalf of Matthias Kurz. The commits include
`Co-Authored-By: OpenAI Codex <codex@openai.com>` per `AGENTS.md`.

## Test plan

- [x] On the first commit alone, the new HTTP/2 sibling-stream test
reproduced shared connection-window starvation: a sibling response did
not progress while the first response remained suspended.
- [x] The focused `ResponseBodyControlTest`,
`Http2ResponseBodyControlTest`, `NettyResponseFutureTest`,
`TimeoutTimerTaskTest`, `SuspensionAwareHttp2LocalFlowControllerTest`,
and `Continue100InterceptorTest` suites pass 38 tests: 12 HTTP/1.1
control tests, 6 HTTP/2 integration tests, 9 future-lifecycle tests, 4
timeout-task tests, 5 flow-controller accounting tests, and 2 HTTP/1.1
Continue tests.
- [x] The flow-controller tests cover unchanged connection accounting
without suspension, connection-only refill during suspension, DATA
received before the suspension callback, overlapping suspensions, and
avoiding double credit after normal accounting resumes.
- [x] `JAVA_HOME=<jdk-11> ./mvnw clean verify`: BUILD SUCCESS for the
full reactor, including tests, Javadocs, coverage, and Revapi.
- [x] The locally published AHC snapshot passes the full Play WS Scala
2.13 and Scala 3.3.8 test matrices on Java 17 and Java 21: 183
integration tests and 76 unit tests pass in each combination, with 2
expected pending tests.
- [x] Play WS code validation, documentation, Scala 2 and Scala 3 MiMa
checks, and dependency-tree verification pass against the locally
published AHC branch.

---------

Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: Aayush Atharva <24762260+hyperxpro@users.noreply.github.com>
hyperxpro added a commit that referenced this pull request Aug 31, 2026
## Summary

- Build keep-body redirects from the original request, preserving every
supported body representation and per-request setting.
- Clear target-specific routing and credential state when a redirect
crosses an origin, including separately stored Cookie objects.
- Preserve an explicit `Content-Length` when replaying a raw
`InputStream` or an unknown-length `InputStreamBodyGenerator`.
- Replay resettable streams and fail promptly for consumed raw streams,
streamed multipart parts, and vanished files that cannot be replayed
safely.
- Pin body bytes, headers, body-selection precedence, caller-owned
`ByteBuf` references, and the new failure modes with focused tests.

## Problem

`Redirect30xInterceptor` rebuilds a request when it follows a strict
302, 307, or 308 redirect. Its keep-body copy chain handled form
parameters, strings, byte arrays, `ByteBuffer`, body generators, and
multipart bodies, but omitted four real request send paths:

- `List<byte[]>` / composite byte arrays
- Netty `ByteBuf`
- `InputStream`
- `File`

The redirected request therefore kept its method and Content-Type but
sent zero bytes. The `File` case is especially risky for uploads because
the target can accept an apparently valid empty PUT or POST.
Reconstructing the request field by field also omitted unrelated
per-request state such as the read timeout and range offset, and
maintaining a second body-selection chain alongside
`NettyRequestFactory` made future drift likely.

This is a pre-existing omission. AHC issue
[#1643](#1643)
previously fixed the same class of bug for multipart bodies. The copy
chain was carried through pull request
[#1843](#1843)
without a policy discussion. Focused searches found no existing issue or
pull request covering these four representations.

## Change

Build a keep-body redirect with `request.toBuilder()` and then replace
only redirect-specific state. This preserves all current and future body
representations and per-request options without duplicating
`NettyRequestFactory.body`. Headers are copied before redirect-only
values are removed, so the original request is not mutated.

On a cross-origin redirect, the copied request drops the previous
resolved address, virtual host, realm, authorization headers, and Cookie
objects before the cookie store adds cookies that legitimately match the
new URI. The body is not covered by that boundary. It follows the
existing keep-body policy, which means a `File` or `InputStream` body
that a cross-origin redirect leg previously received as empty is now
sent in full, and a target that keeps redirecting can receive it once
per hop up to `maxRedirects`. That is the same exposure byte arrays,
strings, form parameters, and multipart bodies already have today.

Composite byte arrays, caller-owned `ByteBuf`s, and files are
repeatable. A resettable `InputStream`, such as `ByteArrayInputStream`,
also replays. A caller-supplied `Content-Length` is retained for a raw
`InputStream` or an `InputStreamBodyGenerator` without a declared
length, because neither has an intrinsic size from which to recompute
it. A consumed stream that cannot be reset reaches the existing
fail-fast guard added in #2312 and completes the future with
`IOException`; that is preferable to silently succeeding with an empty
body.

An `InputStreamPart` is closed by the first multipart send and has no
equivalent replay guard, so a keep-body redirect now fails promptly
instead of risking a hang or incomplete multipart request. A selected
`File` or `FileBodyGenerator` is also checked before dispatching the
redirect; if it disappeared after the first send, the future fails with
`IOException` before a target pooled channel can be removed and an
unchecked constructor exception can escape. The validation follows
`NettyRequestFactory` precedence so a sticky `File` field is ignored
when a higher-priority body representation was actually sent.

The change does not alter which methods or status codes keep a body, nor
does it introduce a new cross-origin policy. It makes the existing
strict-302, 307, and 308 behavior complete for every supported
request-body representation.

## Compatibility

There is no public API change. Requests that previously sent an empty
body on a keep-body redirect now resend their configured body.

**Behavior changes:**

- A non-resettable `InputStream` on a keep-body redirect previously
completed successfully after sending an empty redirected request. It now
completes the request future exceptionally with `IOException`. This
includes `FileInputStream`, which is closed after the first send and
cannot be reset for replay.
- A multipart `InputStreamPart` now fails promptly with `IOException`
when a keep-body redirect requires replay. Reusing its already-consumed
and closed stream could previously hang or send incomplete multipart
content.
- A selected file that disappears between the first request and redirect
now fails with `IOException` before redirect dispatch rather than
allowing an unchecked `IllegalArgumentException` to escape while
constructing the next request.
- A `File` or `InputStream` body is now sent on a keep-body redirect to
a different origin, where the redirected request previously carried no
body. Credentials are still stripped at that boundary, but the payload
is not.

Callers that accidentally relied on an empty or incomplete redirected
request will observe an exception, but the failure is explicit instead
of silently losing configured content. There is no public API change.

## AI disclosure

OpenAI Codex on behalf of Matthias Kurz. The commit includes
`Co-Authored-By: OpenAI Codex <codex@openai.com>` per `AGENTS.md`.

## Test plan

- [x] On untouched `upstream/main`, the focused suite reproduced five
failures: four body types arrived as zero bytes and a non-resettable
stream incorrectly completed successfully.
- [x] Before the generator fix, a one-argument
`InputStreamBodyGenerator` sent `Content-Length: 13` on the first leg
and no `Content-Length` on the redirected leg.
- [x] `./mvnw -pl client
-Dtest=RedirectBodyTest,RedirectCredentialSecurityTest test` on JDK 11:
40 tests passed, including Netty leak detection.
- [x] `./mvnw clean verify` on JDK 11: 1,496 tests passed and Revapi
completed without failures (`BUILD SUCCESS`).

Generated with OpenAI Codex.

---------

Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: Aayush Atharva <24762260+hyperxpro@users.noreply.github.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.

1 participant