Github user rschmitt commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@ok2c I've been meaning to circle back on this discussion. I did a few
hours of work on this problem and got the majority of the integration tests
passing with my alternate backpressure interface, but the changes are
definitely invasive and destabilizing, simply because of how widely implemented
these interfaces are within the codebase; I think that changing these
interfaces requires either more time or more experience with the codebase than
I currently have. However, since these are *user*-facing interfaces that we'll
be stuck with after release, I think we should seriously consider biting the
bullet and making the changes.
The interface I've been working with is:
```java
public interface AsyncDataConsumer extends ResourceHolder {
int initCapacity(CapacityChannel capacityChannel);
void consume(ByteBuffer src) throws IOException;
void streamEnd(List<? extends Header> trailers) throws HttpException,
IOException;
}
```
This interface implements the three points I suggested above, and is
conceptually equivalent to the Reactive Streams API (where the `initCapacity`
method is analogous to `onSubscribe`, and the `CapacityChannel` is the
`Subscription`).
To address the points you made one by one:
> The current API has been designed to work well with both HTTP/1.1 and
HTTP/2.0. For instance, the initial capacity is out of control for individual
HTTP/2 consumers. They must be prepared to consume at least the initial amount
of data defined by HTTP/2 settings during the HTTP/2 protocol handshake.
This is true, but I think it can be addressed simply by decoupling
buffering and processing. Currently, if processing looks like:
`AbstractHttp2StreamMultiplexer -> Buffering processor`
We can simply refactor to:
`AbstractHttp2StreamMultiplexer -> Per-stream buffer -> AsyncDataConsumer`
This structure does a better job of separating internal concerns
(per-stream buffering) and user concerns (the actual data and how it is
processed), and makes it easier (fewer surprises) for users to implement the
`AsyncDataConsumer` directly if they choose to do so.
> The capacity value returned by #consume method was necessary to make the
same API work with HTTP/1.1 connections that do not even support the concept of
data capacity.
I don't see how that follows. It's obviously true that HTTP/1.1 has no
explicit windowing, but it's still an inherently asynchronous protocol, by
virtue of the fact that it runs on top of TCP. You either read from the socket
or abstain from reading from the socket, depending on your ability to process
data (which is signaled via an asynchronous backpressure mechanism).
> There is no easy way for HTTP/1.1 consumers to make the event handler
aware of the fact that they are not able to process any more input without data
loss.
In reactive streaming, we signal exactly how much more data we *can*
process; we don't explicitly tell the producer to *stop*, and the producer
never sends us more data than we requested.
> The only alternative I can think of using different capacity interfaces
for different HTTP protocol versions which would be terrible and hardly any
simpler.
That would be terrible, but I think that protocol-specific buffering
techniques (internal to the client) are all we really need here. The
backpressure strategy I'm describing is one that I've implemented for both
HTTP/1.1 and HTTP/2 on top of Netty; it works fine.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
