Review of new Http client API

[Michael McMahon]

On 22/08/12 15:29, Chris Hegarty wrote:
> Michael what you have looks good.
>
> But, I think what Sam is suggesting ( or maybe not, but I like it ;-) 
> ), is something like this. (I'd need to think more about what effect 
> this has on the different modes, async/blocking )
>
>  class HttpResponse {
>    HttpResponse onHeaders(Block<HttpResponse>);
>    HttpResponse onError(BiBlock<HttpResponse,Throwable>);
>    HttpResponse onBodyPart(BiBlock<HttpResponse,ByteBuffer>);
>  }
>
Right. I see what you mean in terms of making the setting of the callbacks
fluent. I assume that Block and BiBlock are types associated with Lambda 
somehow,
but otherwise this is unfamiliar territory to me ....
>  response.onHeaders(r -> headers(r))
>          .onError((r,t) -> error(t))
>          .onBodyPart((r,bb) -> body(r, bb));
>
So,  headers() and body() would be methods on HttpResponse ... Right ?
What is error()?

- Michael.
> Alternatively, I believe something like this would also be compatible 
> with lambda (since there is a default implementation for on Error):
>
>   interface HTTPResponseHandler {
>     public void onHeaders(HttpResponse resp);
>
>     public void onError(HttpRequest request, Throwable exception)
>     default { throw exception; }
>  }
>
> -Chris.
>
>
> On 21/08/2012 14:57, Michael McMahon wrote:
>> Sam,
>>
>> Thanks for the comments. Some discussion below.
>>
>>
>> On 17/08/12 00:13, Sam Pullara wrote:
>>> I suggest that you make it a more fluent API rather than having
>>> multiple callback methods in your callback interface. As it stands it
>>> isn't compatible with lambdas. You might take some inspiration for the
>>> asynchronous callbacks from my work porting Twitter's Future/Promise
>>> to JDK8:
>>
>> I agree with the above. In a previous version of the API the main
>> callback was lambda compatible.
>> Originally we used HttpResponse to encapsulate everything related to a
>> response
>> including errors. But, some preferred to keep HttpResponse aligned to an
>> actual response
>> from a server in all cases. There might be other ways to get around that
>> by combining
>> HttpResponseHeadersHandler.{onError(), onHeaders()} back into a single
>> method.
>> Maybe, drop the onError() method and add the exception/throwable as a
>> parameter to onHeaders()
>>
>> But, we also wanted to provide notification of body data (through the
>> sub-interface HttpResponseHandler).
>> Keeping the two interfaces distinct meant that applications could get
>> asynchronous notification of
>> the response headers, but then possibly read the response body in a
>> blocking manner.
>> Or alternatively, applications can use the handler to be notified of
>> both headers and body.
>>
>> So, if we revert HttpResponseHeadersHandler back to having a single
>> method, the sub-interface
>> now would have two methods (instead of three).
>>
>> One way around that could be to have two unrelated interfaces:
>>
>> interface HttpResponseHeadersHandler {
>> public void onHeaders(HttpResponse response, Exception e);
>> }
>>
>> interface HttpResponseBodyHandler {
>> public void onBodyPart(HttpResponse resp, ByteBuffer buffer, boolean 
>> last);
>> }
>>
>> // Then a HttpResponseBodyHandler would be added to
>> HttpClient.sendRequest() as below:
>>
>> public void sendRequest(HttpRequest, HttpResponseHeadersHandler,
>> HttpResponseBodyHandler);
>>
>>
>> Both of the interfaces would be lambda compatible (again) though at the
>> cost
>> of having to specify two separate handlers. So, the following might 
>> be how
>> it could be used (and using a builder for HttpClient)
>>
>> HttpClient client = HttpClient.createBuilder()
>> .setAsynchronousChannelGroup (..)
>> .setCookieManager(..)
>> .setDefaultTimeout(..)
>> .setProxy(...)
>> .addFilter(...)
>> .buildClient();
>>
>> HttpRequest request = client.createRequest(new 
>> URI("http://www.foo.com/"))
>> .setBody("Hello world".getBytes())
>> .setMethod(HttpMethod.POST);
>>
>> client.sendRequest (
>> request,
>>
>> // handle headers
>> (HttpResponse response, Exception e) -> {
>> if (response.getResponseCode() != 200) {
>> // handle error response
>> }
>> // handle normal case
>> },
>>
>> // handle body
>> (HttpResponse response, ByteBuffer buf, boolean last) -> {
>> // handle data in buf
>> }
>> );
>>
>> It seems fairly readable still, I think.
>>
>> Another thing that this usage points to, is the usefulness of being able
>> to hang some user context
>> off of the HttpResponse or HttpRequest objects. That would be the only
>> way to share some user state
>> between the two handlers above, in this Lambda style.
>>> https://github.com/spullara/java-future-jdk8
>>>
>>> Another consideration might be to make sure that it is compatible with
>>> an implementation that is using SPDY under the covers for connectivity
>>> as I suspect that HTTP as a wire protocol has peaked though the HTTP
>>> semantics will survive.
>> Right. This is important. One area where there will be changes is with
>> pipe-lining.
>> We need to ensure that our pipe-lining API is not restricted to only
>> Http 1.1 pipe-lining
>> Are you aware of other areas that could have an impact on the API?
>>
>> Thanks
>> Michael.
>>
>>> Sam
>>>
>>> On Tue, Aug 14, 2012 at 5:01 AM, Michael McMahon
>>> <michael.x.mcmahon at oracle.com> wrote:
>>>> Hi,
>>>>
>>>> (apologies for sending this again)
>>>> We have just published a draft of a proposed new Http client API [1]
>>>> for JDK
>>>> 8.
>>>>
>>>> This message has been cc'd to jdk8-dev so that as many people as
>>>> possible
>>>> know about it, but the discussion will be on the net-dev list
>>>> (net-dev at openjdk.java.net).
>>>> So, folks will have to join that list [2], in order to take part.
>>>>
>>>> Thanks,
>>>> Michael.
>>>>
>>>> [1]http://cr.openjdk.java.net/~michaelm/httpclient/v0.3/
>>>>
>>>> [2]http://mail.openjdk.java.net/mailman/listinfo/net-dev
>>
>>