Hi, On Wed, Mar 21, 2018 at 9:33 PM, Chris Hegarty <chris.hega...@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
