Skip to content

Issue/544 reactive streams support - #963

Merged
slandelle merged 1 commit into
AsyncHttpClient:masterfrom
dotta:issue/544-reactive-streams-support
Sep 7, 2015
Merged

slandelle merged 1 commit into
AsyncHttpClient:masterfrom
dotta:issue/544-reactive-streams-support

Conversation

@dotta

@dotta dotta commented Sep 1, 2015

Copy link
Copy Markdown
Contributor

Here is an initial proposal to fix #544.

I'm entirely new to both Netty and AHC, so if you notice anything weird it's very likely that I haven't understood something :-)

Other than that, something is puzzling me with the tests I wrote. Basically, if I change the CHUNK_SIZE in https://github.com/AsyncHttpClient/async-http-client/compare/AsyncHttpClient:master...dotta:issue/544-reactive-streams-support?expand=1#diff-5133f4e3fed98f7a99081a1ae643c4c0R104 to a value around 20000 (or lower), the tests start to fail because there is a mismatch between the received bytes and the expected ones. I honestly have no good explanation for this, but I thought it may have something to do with the internals of Netty (or maybe a config flag?). I'd really appreciate it if someone more experienced with Netty could have a look.
EDIT: This issue is now fixed after using RxJava instead of providing my own implementation of Publisher in the test :-)

This PR also includes @jroper commit to add support for handling response bodies.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking, really?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative is queuing up elements in the subscriber, but the risk with that is overflowing the queue (dropping elements is even worse) when the consumer is much slower than the producer. Also, note this implementation is only used when the provider doesn't support reactive streams.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, note this implementation is only used when the provider doesn't support reactive streams.

I'd rather throw an UnsupportedOperationException then. :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, I'm looking at the implementation of FeedableBodyGenerator and realise I probably don't need blocking. Forgive me :-)

@slandelle

Copy link
Copy Markdown
Contributor

Hi Mirko,

I think your ReactiveStreamBodyGenerator is broken. Are you sure it cannot be implemented on top of FeedableBodyGenerator?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the buffer doesn't have an array?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is that possible? Any idea how I should handle that? (should I create the array myself, or fail?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is that possible?

Of course: DirectBuffers, composites, etc. That's what hasArray() is for.

See FeedableBodyGenerator, you can put a ByteBuffer into another.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dotta the netty in action cover this:).composites could have array if it have only one element and the element have array or it was empty.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hepin1989 Don't confuse Netty's ByteBuf and Java's ByteBuffer :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slandelle haha,sorry,I don't see the whole context:).

@dotta

dotta commented Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

Hi @slandelle, thanks a lot for the feedback!

I think your ReactiveStreamBodyGenerator is broken. Are you sure it cannot be implemented on top of FeedableBodyGenerator?

Do you mean the implementation of ReactiveStreamBodyGenerator.StreamedBody? After looking at the FeedableBodyGenerator.PushBody, I feel like I should be able to reuse it. I'll have a deeper look now.

@slandelle

Copy link
Copy Markdown
Contributor

Do you mean the implementation of ReactiveStreamBodyGenerator.StreamedBody? After looking at the FeedableBodyGenerator.PushBody, I feel like I should be able to reuse it. I'll have a deeper look now.

Yeah, they serve the same purpose, so I guess existing code could be reuse/extended/wrapped. FeedableBodyGenerator.PushBody currently has a known issue that the reading buffer has to be larger than the payload, but that's something that can/should be fixed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jroper I'm not so convinced about requesting only one element at a time. Any thought?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering this is a fallback implementation, I think it's fine. Another option, seeing as the provider is going to end up buffering infinitely anyway, is you can invoke subscription.request(Long.MAX_VALUE) in onSubscribe, this is allowed according to reactive streams spec 3.17.

@dotta

dotta commented Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

@slandelle I think you should be more pleased now (I for sure am :-)).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, I'd rather remove the I prefix and prefix the default implementation with Simple (or better if you can think of something).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change copyright to "the AsyncHttpClient Project".

@dotta
dotta force-pushed the issue/544-reactive-streams-support branch from a7b7b08 to f91bac1 Compare September 2, 2015 14:58
@dotta

dotta commented Sep 2, 2015

Copy link
Copy Markdown
Contributor Author

@slandelle I believe I've addressed all comments. The only pending one is #963 (comment), which you are discussing with @jroper.

Let me know if you have other comments, or if I should rebase and squash commits.

@slandelle

Copy link
Copy Markdown
Contributor

@dotta @jroper Sorry, busy day. Will have a look this week-end.

@dotta

dotta commented Sep 7, 2015

Copy link
Copy Markdown
Contributor Author

@slandelle I'm happy to squash commits and rebase if you give me your bless :-) In the meanwhile, I'll update Play master to take advantage of the changes in this PR. Whohoo!

@slandelle

Copy link
Copy Markdown
Contributor

@dotta Sorry, had some unexpected things to deal with first... Yes, could you please squash and rebase? Let's merge this so everyone can move forward and get everything stable in the next weeks so we can release 2.0.0.

@dotta

dotta commented Sep 7, 2015

Copy link
Copy Markdown
Contributor Author

Whohooo! I'm on it, thanks! :-)

@dotta
dotta force-pushed the issue/544-reactive-streams-support branch from a373a21 to 71c658c Compare September 7, 2015 13:49
@slandelle slandelle added this to the 2.0.0 milestone Sep 7, 2015
slandelle added a commit that referenced this pull request Sep 7, 2015
@slandelle
slandelle merged commit fc870d8 into AsyncHttpClient:master Sep 7, 2015
@dotta
dotta deleted the issue/544-reactive-streams-support branch September 7, 2015 14:15
@slandelle

Copy link
Copy Markdown
Contributor

@jroper @dotta Thanks guys!

@dotta

dotta commented Sep 7, 2015

Copy link
Copy Markdown
Contributor Author

@slandelle Thank you for merging!

I'm updating the Play codebase to use AHC master, will let you know if I hit any issue.

@slandelle

Copy link
Copy Markdown
Contributor

Great!
What's your timeline for Play 2.5?

@dotta

dotta commented Sep 7, 2015

Copy link
Copy Markdown
Contributor Author

I believe it's November 2015, but I'll let @jroper confirm.

Once I've verified that things work properly on the Play side, I'd really appreciate if you could release a 2.0.0 milestone (in case you would like to wait a bit more before cutting a 2.0.0 final).

@slandelle

Copy link
Copy Markdown
Contributor

I can easily cut an alpha.

I'm still considering some refactorings (maybe Proxies) + I might want to break a few things to improve DNS handling in Gatling.

@dotta

dotta commented Sep 7, 2015

Copy link
Copy Markdown
Contributor Author

Sounds good. I'll ping you soon :-)

@dotta

dotta commented Sep 8, 2015

Copy link
Copy Markdown
Contributor Author

@slandelle I've a branch in Play that is now using the new StreamedAsyncHandler, and it seems to be working just fine (and I'm really pleased with the result, the code is much, much, much easier to read now!). Could you cut a release so that I can push that work in Play?

@slandelle

Copy link
Copy Markdown
Contributor

I just released 2.0.0-alpha9.
Cheers!

@jroper

jroper commented Sep 8, 2015

Copy link
Copy Markdown
Contributor

Thanks @slandelle!

@dotta

dotta commented Sep 9, 2015

Copy link
Copy Markdown
Contributor Author

@slandelle Thank you!

@mkurz mkurz mentioned this pull request Aug 26, 2026
6 tasks
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Backpressure in AsyncHandler

4 participants