RFR: 8087112 HTTP API and HTTP/1.1 implementation

[Michael McMahon]

Hi Paul

Thanks for the review. Some comments below


On 17/02/16 15:26, Paul Sandoz wrote:
>
>> On 4 Feb 2016, at 17:14, Michael McMahon 
>> <michael.x.mcmahon at oracle.com <mailto:michael.x.mcmahon at oracle.com>> 
>> wrote:
>>
>> Hi,
>>
>> The following webrevs are for the initial implementation of JEP 110.
>> Most of it is in the jdk repository with some build configuration in 
>> the top
>> level repo for putting this code in its own module (java.httpclient).
>>
>> http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/ 
>> <http://cr.openjdk.java.net/%7Emichaelm/8087112/01/top/webrev/>
>>
>> http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/
>>
>
> I am mostly focusing on the API, because that is where i have had most 
> input in the past. Over all the API is quite compact, appears easy to 
> use for common cases, yet has a powerful plugin mechanism for 
> request/response bodies, and the design allows for but the 
> implementation does not have to provide a fully non-blocking 
> implementation.
>
>
> HttpHeaders
> —
>
>    61     public Optional<Long> firstValueAsLong(String name);
> Why not return OptionalLong?
>

Okay. I've added this to JDK-8134652 which is tracking small
spec. changes.

> The implementation class HttpHeadersImpl does not appear to 
> support unmodifiable values i.e. the list obtained from the header map 
> can be modified. Also might need to be careful about clone, since that 
> can be called by the user, and this is a shallow clone.
>
Right. The map and any component lists should be unmodifiable.
Will fix this.

>
> HttpRequest
> —
>
> In the examples recommend using URI.create rather than new URI.
>
> The examples section can be placed under an @apiNote
>

ok, will add this for spec change also

> When building a request how does one set multiple values for a header? 
> setHeaders overwrites, does one used headers(…) instead?
>

headers(String, String)
and headers(String ...) both accumulate headers

>
> Processors
> —
>
> I think the flowController of HttpRequest.BodyProcessor.onRequestStart 
> requires more specification (a follow up issue if you wish):
> - what are the units? It represents unfulfilled demand, but of what?
> - what if a -ve value is passed
> - what if Long.MAX_VALUE is passed? effectively unbounded?
> - does it have to be called within onRequestStart and onRequestBodyChunk?
> - what happens if it is invoked outside the life-cycle of a BodyProcessor?

yes, spec that is in HttpResponse.BodyProcessor applies here too,
but it should be repeated.

> OK, i see there is more specification in HttpResponse.BodyProcessor. I 
> wonder how implementations correlate the unit to bytes? Is there any 
> correlation related to an internal buffer size? Why would the 
> requested unfulfilled demand be greater than 1? i.e. as an implementor 
> of a body processor how am i meant to correlate the bytes i read/write 
> to the units required to express the unfulfilled demand?
>
There is no direct correlation between units and bytes. If the window 
size is 1, then the processor
is willing to accept one more call, of whatever sized ByteBuffer.

The implementor of the processor must then decide based on how long it 
takes to consume the
data whether to update the window immediately or to delay and update it 
later.

> For responses, what happens if the Consumer<Buffer> is called outside 
> the life-cycle of the processors?
>

Actually, that is not part of the current API. For the moment, the 
life-cycle of the ByteBuffer is
that it only belongs to the processor for the duration of the call, 
meaning it would have to be
copied if the data isn't consumed fully during the call.

Thanks!

Michael
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160218/eb4ad258/attachment-0001.html>