Review of new Http client API

[Michael McMahon]

On 22/08/12 22:09, Sam Pullara wrote:
> On Aug 22, 2012, at 1:17 PM, Chris Hegarty wrote:
>> On 22/08/12 21:05, Michael McMahon wrote:
>>> 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 )
> Yep, this is exactly what I was suggesting. Thanks for clarifying with code.
>
>>>> 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
>> Right, these are types defined in the lambda repo, that will most likely be part of JDK8. Worth a look, and we should also confirm with the lambda folks before build them into this API.
> Briant Goetz would be the right person to ask. In the meantime, there shouldn't be an issue using custom interfaces with a single method (HeaderCallback, ErrorCallback, BodyCallback, etc).

Yes, I think that gives us a good fall back in case things develop 
differently in Lambda. For the moment, I will use three
different interfaces, and put some text in the apidoc suggesting they 
might be replaced with standard Lambda types
before JDK8 is finished.

Thanks
Michael.

> Sam
>
>>> 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()?
>> No, sorry for the confusion. headers(), error(), and body() are just examples of what some user code could be ( rather than cluttering the example with too much code ).
>>
>> -Chris.
>>
>>> - 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
>>>>>