Hi Simone, Thank you for your reply.
I have inserted inline my replies to specific assertions and comments, but please continue to read on past these as there two concrete proposals further down that address your concerns, as I understand them. > On 30 Mar 2018, at 14:34, Simone Bordet <simone.bor...@gmail.com > <mailto:simone.bor...@gmail.com>> wrote: > > ... > 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. I can find no material supporting this assertion on reactive-streams.org <http://reactive-streams.org/>, or anywhere else for that matter. This is not something that I have, or any member of the HTTP Client team has, ever come across. We have had review comments and feedback, on this list, from folk in the Reactive Streams community ( that greatly improved interoperability ), and this has never come up. Without supporting material, I cannot accept this assertion. Publisher and Subscriber are low level building blocks that can be used by different Reactive Streams (RS) implementations. They were added to Java SE 9 to provide interoperability between various RS implementations. The HTTP Client API, being proposed for inclusion in Java SE 11, can only directly reference other Java SE types, so uses Publisher and Subscriber directly. The HTTP Client API provides out-of-the-box Publisher and Subscriber implementations for various common operations. Beyond that, custom implementations can be used, or more likely Publishers and Subscribers from RS implementations. Interoperability with these RS implementations is of utmost importance. At the end of a Flow, there must always be a Subscriber. Without it there is no demand. Therefore, all RS APIs must provide Subscribers of some sort or other. > 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; I am surprised by this assertion. Our experience writing Publishers and Subscribers has shown that the most difficult piece of code to write, in a scalable thread-safe manner, is the Subscription::request. Subscriptions are created and returned by a Publisher and usually have an intimate relationship with their Publisher. I consider them part of the Publishing-side. If a Publisher is not creating its own Subscription, but instead returning a Subscription from an upstream Publisher, in a chain, then some work can be avoided, but this is not always the case. I believe that this could be the scenario that you are referring to, but it is not always the case. The HTTP Client deliberately does not expose its Publisher of response body data. It instead subscribes the given Subscriber, on behalf of the invoking code. There can, and must, be a single Subscriber. This is a simple and safe model, expected to be more readily understood by an average Java developer ( than exposing a Publisher ). If Publishers are better for interoperability with some RS implementations, then this could be addressed by a small addition to the API, as proposed later in this mail (see below). > Processors even > more so, so it is better that this task is left to library > implementers such as the JDK, Reactor and RxJava2. Again, similar to Publisher, these are highly context sensitive. For some common "pass-through" scenarios then they can be relatively straight forward, but not so when processing is required. I agree, I think the average Java developer should not be expected to write these, for common scenarios. > 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. The HTTP Client has had this design for a couple of years now, and the use of Subscriber ( of response body data ) has not surprised anyone, to my knowledge. Again, folk in the Reactive Streams community are aware of this API. > 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). In many cases a simple single Subscriber ( sink ) will be a reasonable choice. The HTTP Client provides a number of combinators and adapters that demonstrate the composability of Subscribers. For interoperability with other RS implementations, where the Flow adaptation is being done through Publishers, I would expect folk to use something like RxJava2's `io.reactivex.processors.PublishProcessor`. In fact, Daniel just wrote something similar in a recent test [1], a handler that makes the response body available through a Publisher, `BodyHandler<Publisher<List<ByteBuffer>>>`. It is a not a general purpose Processor, but a Subscriber that makes its data available through a Publisher. > 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. Sorry, the read-side and write-side are just different ( but use the appropriate consumer and producer API from RS ). I don't see this as an asymmetry. One can use the aforementioned `PublishProcessor` if echo is required. > 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. Already answered above. > 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. If one is using a RS API then maybe this would be easier, but not so for other cases. I content that writing Publisher is not as easy as a Subscriber, and we already know that a Subscriber is required. Such an API would require developers, not using an RS API, to have to write a Publisher and a Subscriber. > Utility Publishers that currently discard, convert to string, save to > file, etc. can be equally trivially implemented as they are now in the > JDK. Yes, probably, but doesn't provide anything more than we already have. I don't believe that this is a simplification or improves interoperability in any way. The one area that I do see some room for improvement is the possibility of providing a Publishing handler out of the box, so one does not need to reach beyond Java SE for such echo use-cases. Daniel already has this code in the repository as a test [1], so it would not take much work to provide an API similar to: BodyHandlers { .. public static BodyHandler<Publisher<List<ByteBuffer>>> ofPublisher() {...} .. } Does this satisfy your concern? --- > 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. We have considered similar ( a data carrier rather than explicit parameters ) a number of times, but discounted it, since we found no use-cases for additional information, and none have come to light. Is a carrier, its instantiation per response, API footprint, warranted? To date, we have not found it a limiting factor, but I agree that it would be somewhat easier to evolve the API with such a carrier. > 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. What use case have you in mind for this? > So perhaps: > > public Publisher<T> apply(HttpResponse response, Publisher<ByteBuffer> > responseContent); > > This change would also simplify the maintenance since BodySubscriber > will be removed. Your sketch API design throws out `BodySubscriber` that is at the core of the whole API, it is responsible for conversation of response body bytes to the higher-level object, and control flow through its `CompletionStage` ( I don't see how you address control flow in your sketch ). What you are suggesting is a significant change, not prototyped, not incubated, not easier to use, and from what I can see brings no improvements. You are proposing an alternative design based on your assertions above ( some of which I disagree with ). I accept that a carrier would be a more friendly for possible evolution, maybe: BodyHandlerInfo { int statusCode() {... } HttpHeaders responseHeaders { ... } } Does this satisfy your concern? --- > 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. No one is forcing anyone outside of RS APIs. They already support Subscribers and Publishing Processors. They already have what is needed. I have made two concrete proposals in this email that attempt to address concerned that you have raised. Both represent improvements while staying within the current design. Do these satisfy your concerns? -Chris. [1] http://hg.openjdk.java.net/jdk/sandbox/file/c59f684f1eda/test/jdk/java/net/httpclient/ResponsePublisher.java <http://hg.openjdk.java.net/jdk/sandbox/file/c59f684f1eda/test/jdk/java/net/httpclient/ResponsePublisher.java>
