Hi,
On Wed, Apr 6, 2016 at 4:08 PM, Michael McMahon
<michael.x.mcma...@oracle.com> wrote:
> Hi,
>
> This is the webrev for the HTTP/2 part of JEP 110.
>
> http://cr.openjdk.java.net/~michaelm/8087124/webrev.1/index.html
API
I have already mentioned in 2 previous messages my feedback about the
API, but a few of my concerns have not been addressed, so here they
are again:
* HttpRequest.create(URI) and related.
I don't think a URI is suitable for OPTIONS or CONNECT, although you
can actually build one.
IMHO needs a method Builder.target(String) to accommodate such cases,
and some facility to create a proper URI (especially in case of query
parameters that often need to be URL encoded).
* HttpClient/HttpRequest/etc. getters.
Getter methods should follow the JavaBean standard to comply with
introspective libraries (e.g. JMX) and tools. E.g.
HttpRequest.method() -> HttpRequest.getMethod() and so forth.
I am fine dropping the "set" for fluent interfaces such as Builder,
but IMHO getters should have the "get".
* HttpRequest.BodyProcessor and HttpResponse.BodyProcessor method
naming consistency.
Some method contain the word "request" (or "response"), some don't,
some contains the word "body", some don't, etc.
* HttpRequest.Builder.headers(String...) way too confusing.
Especially now that there are header() and setHeader(); it's confusing
when trying to set headers with multiple values:
headers("Accept-Encoding", "gzip", "deflate") does not do what is
intuitively expected.
Also, it's not clear the semantic when the same header is present more
than once: headers("Foo", "a", "Foo", "b") has add or replace semantic
?
I would remove this method, does not bring any benefit, it adds
confusion, and the other 2 are already chainable and with clear
semantic.
* HttpRequest.Builder.timeout(TimeUnit, long).
The argument order is the opposite of *all* the other usages in the
JDK, I think it should be made conformant. Using (long, TimeUnit) also
reads (5, SECONDS) much better than (SECONDS, 5).
* HttpClient.Builder.pipelining().
Relic of the past, remove it.
* No per-request cookie support.
Manually setting the Cookie header is prone to errors (quoting, paths, etc.)
* HttpClient.Builder.priority().
Not sure how this can be used (not for HTTP/2 despite the javadocs).
HTTP/2 "priority" is not really a priority, it's a dependency from
another request.
I don't see how this API can be used at all.
* MultiResponse.
Neither HTTP nor HTTP/2 have "multiple responses" (well perhaps only
for 100 and 101 codes).
I would rename this functionality into something like PushHandler, and
separate it from the receive of the main response.
In this way, HTTP/1.1 requests may avoid to add the PushHandler, while
HTTP/2 requests may decide whether they are interested or not in
pushes.
The code that handles the main response remains unchanged.
* Aborting the request
I don't see any method to abort a request or response ?
* HttpResponse.BodyProcessor.onResponseBodyChunk()
The javadocs of this method states that "ByteBuffers can be reused as
soon as this method returns", but I think this is wrong because it
forces applications to copy the data in case of async consumption.
BodyProcessor has a flow control mechanism that tells when the
application is done with the data; before the flow control requests
another buffer, it should be assumed that the buffer is still in use.
For example:
void onResponseBodyChunk(ByteBuffer buffer) {
writeAsyncToDisk(buffer, new CompletionHandler() {
success: longConsumer.accept(1);
failure: ??? what here ?
});
}
In the failure case, how would an application abort (e.g. send a
RST_STREAM to the server) ?
IMPLEMENTATION
In general, I find this implementation disappointing because it is not
non-blocking.
Concepts from ReactiveStreams (JDK9's Flow) have been borrowed, but
then turned into blocking code.
Every connection uses 2 threads, plus additional threads spawned to
achieve asynchronous behavior.
Any call to the "async" versions of the API is actually spawning a new
thread and block in that thread.
Like for the WebSocket case, I don't see the reason to use all these
resources, when a non-blocking implementation would have been simpler
and used much less resources.
I think this is especially important for code to be included in the
JDK, so I think there is a room for a lot of improvement here.
The details:
* HttpRequest.BodyProcessor
Flow control is not implemented for HTTP/1.1, but it needs to,
otherwise it breaks async HttpRequest.BodyProcessor implementations.
* HttpRequest.BodyProcessor.onComplete()
This method is never invoked by the implementation ?
* Stream.responseBody()
In case onResponseBodyStart() returns null, the body is read
asynchronously, but then responseReceived() is called concurrently
with the async read of the body ?
And then called again from receiveDataAsync() ?
* Window updates
They are inefficiently sent for every DATA frame, and in advance.
See above, I think the "consumption" of a buffer from
onResponseBodyChunk(ByteBuffer) does not happen until the flow
controller is notified (not when the method returns), so only when the
flow controller is notified a window update can be sent.
* HTTP/2 writes
Only one frame at a time is being written, which is inefficient. It
would be possible to dequeue multiple frames, generate all the buffers
they require and then gather write the buffers for multiple frames.
In Stream.getDataFrame() the buffer that is being passed to
onRequestBodyChunk() is "limited" to some max payload length, but of
course the application is free to change that limit up to the buffer
capacity, so I'm not sure what the purpose of setting the limit.
Sending one frame potentially blocks 3 times: to wait for
onRequestBodyChunk(), to wait for the stream flow control, and to wait
for the connection flow control.
* HttpResponse.body().
This method takes a BodyProcessor, which is basically a listener for
response body events.
However, an application calling body() won't return from body() until
the whole content has been received. I find this confusing.
I would rather remove this method (to keep only bodyAsync()), and
provide a utility class that has a blocking behavior when passed to
bodyAsync(), something like:
BlockingBody<T> bb = ...;
response.bodyAsync(bb);
T body = bb.get();
Thanks !
--
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz
