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.mcma...@oracle.com <mailto:michael.x.mcma...@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
