RFR [11] 8197564: HTTP Client implementation - JEP 321

[Simone Bordet]

Hi,

On Wed, Mar 21, 2018 at 9:33 PM, Chris Hegarty <chris.hegarty at oracle.com> wrote:
> Hi,
>
> This is a request for review for the HTTP Client implementation - JEP 321.

It is my understanding, and that of the ReactiveStreams (RS) people I
spoke with, that both the RS API and the higher level API based on RS
(such as those offered by Reactor and RxJava2) are supposed to expose
to users only Publishers.
This may be counterintuitive at beginning: the typical case of
InputStream to read content and OutputStream to write content are both
represented with Publishers in the RS experience.
The reason behind only offering Publishers is that Subscribers are
more difficult to implement by regular developers; Processors even
more so, so it is better that this task is left to library
implementers such as the JDK, Reactor and RxJava2.

It is therefore a surprise that the the HTTP client uses instead
Subscriber for the response content.
BodyHandler must return a BodySubscriber, which is-a Flow.Subscriber.

This will force users that need to write non-trivial response content
handling to implement a Subscriber, or most of the times a Processor
(to convert the Subscriber back into a Publisher for consumption by
other more "regular" RS APIs).
It also creates an asymmetry between read and write side, so that for
example it is not possible, without writing a processor, to echo a
response content as the next request content.
Another difficult example is piping the response content to a
WebSocket RS API (that would take a Publisher), and so on for
basically all RS libraries out there.

I would like to suggest to review this: it is definitely possible to
have BodyHandler refactored in this way:

public Publisher<T> apply(int statusCode, HttpHeaders responseHeaders,
Publisher<ByteBuffer> responseContent);

Returning a Publisher that performs transformations on the
responseContent Publisher is the heart of libraries such as Reactor
and RxJava2, so this will be super simple for users of the API.
Utility Publishers that currently discard, convert to string, save to
file, etc. can be equally trivially implemented as they are now in the
JDK.

With the occasion, I think that the statusCode parameter and the
responseHeaders parameter can be replaced by a single HttpResponse
parameter, which would be better for future maintenance in case other
information needs to be provided.
The simple case that comes to mind is the HTTP version of the
response, which may be different from that of the request and
indicates server capabilities: this is currently not provided in the
BodyHandler.apply() signature.

So perhaps:

public Publisher<T> apply(HttpResponse response, Publisher<ByteBuffer>
responseContent);

This change would also simplify the maintenance since BodySubscriber
will be removed.

My concern is that driving users of the JDK APIs outside of the
familiar RS APIs offered by Reactor and RxJava2 where only Publishers
are relevant, by forcing them to implement Subscribers and Processors
(heck, I even have an IntelliJ warning telling me that I should not do
that!), will cause confusion and be potential source of bugs - we all
know non-blocking/async programming is hard.

Thanks !

-- 
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz