Hi Anthony,
Comments/answers below
On 21/03/15 10:44, Anthony Vanelverdinghe wrote:
Hi Michael
Please find my feedback on the current API docs below. It's divided in
3 parts: API (mostly questions), documentation (mostly suggestions for
clarifications) and spelling.
Thanks in advance for your consideration.
Kind regards, Anthony
=== API ===
* will there be support for HTTP 2.0 stream prioritization? For
example by providing a method requestHttp2(int priority) in
HttpClient.Builder/HttpRequest.Builder? This way one could create 2
clients with a different priority: a low-priority one for background
downloads of documents/large files, and a high-priority one for status
updates. Then the 2 clients would use the same TCP connection, but
different streams with different priorities.
I think setting a default priority could be a useful first step in that
direction, so long as we don't preclude full support of stream priority
in the future,
which includes per-request priority and inter-request dependencies. I
think these would be settable per-request only, but the default would be
on the client.
* which convenience methods will be added to existing classes, if any?
I'm particularly thinking of URL, which already has
openConnection/openStream convenience methods for the old API.
I hadn't planned adding any convenience methods to other classes.
* how is compression (the
Accept-Encoding/Content-Encoding/Transfer-Encoding headers) handled?
Is this handled transparently by the HttpClient? For example, if I
request a text document, will the client automatically send a header
"Accept-Encoding: gzip, deflate" (this is the current default in
Firefox)? And once I receive the response, transparently decompress
it, so I can just do "response.body(asString())"?
It wouldn't be transparent as such. But, it should be possible to
implement it using HttpRequestBodyProcessor and
HttpResponseBodyProcessor, but I wasn't
planning to do it as part of this work initially. Someone could define a
class like:
class Deflator<T> implements HttpResponseBodyProcessor<T> {
public Deflator(HttpResponseBodyProcessor<T> p) {..}
:
}
So Deflator takes another response processor and you might use it as:
String body = response.body(new Deflator<String>(asString()));
or something similar.
* how is caching handled? Is there anything analog to "useCaches" in
java.net.URLConnection?
That is an omission in the current API. We need to add setting/getting
of a java.net.ResponseCache to HttpClient(builder)
* HttpClient.Builder: concerning "followRedirects", I feel there are 2
more options that ought to be possible to be set: "same-protocol"
(i.e. the current java.net.HttpURLConnection behavior) and "secure"
(all redirects are allowed, except for redirects from https to http).
A possible solution might be to introduce a "RedirectPolicy" enum with
4 constants: NEVER, ALWAYS, SAME_PROTOCOL, SECURE. Then the method
would become "followRedirects(RedirectPolicy policy)".
That seems useful and exploits something the old API couldn;t support
given that HttpsURLConnection was a subtype of HttpURLConnection
* HttpClient.Builder: the documentation for "requestHttp2" says: "If
that fails, then following requests will not attempt to upgrade
again." How is it determined that the upgrade to HTTP/2 fails? For
example, is the client able to distinguish "the upgrade to HTTP/2
failed" from "some error occured which caused the request to fail"
(e.g. the server is down)? And how can I force to attempt to upgrade
again, for a specific origin and/or all origins? For example in the
case of application servers, having to restart the JVM just to have
the HTTP/2 upgrade to be attempted again, seems rather harsh.
There is some vagueness there, which I hoped to clarify after
implementation starts. The upgrade "failing" just meant
either the ALPN negotiation failing or in the case of h2c, that the
server responds with a regualr HTTP/1.1 response (rather than
a 101 Switching Protocols response). These mechanisms seem to be quite
well defined in the spec and have nothing
to do with other server errors (such as transient network errors)
I think the upgrade should be attempted once per HttpClient instance. If
it fails once, it's not likely to succeed soon again.
But a new HttpClient instance can be created to retry if desired.
* HttpHeaders: why is the method "firstValueAsInt" and not
"firstValueAsLong"? If I want to download e.g. the .iso file of Oracle
Linux, the value of Content-Length will be too big and a
NumberFormatException will be thrown.
Good point.
* HttpHeaders: why is there only a special case for "firstValueAsInt"?
Personally, I would like some more methods to be added:
** java.time.OffsetDateTime firstValueAsDate(String name,
Supplier<OffsetDateTime> defaultValue) which parses headers such as
Expired & Last-Modified, using the datetime format recommended by RFC
7231 (i.e. the format defined in RFC 5322)
** javax.activation.MimeType firstValueAsMimeType(String name,
Supplier<MimeType> defaultValue) which parses headers such as
Content-Type & Accept-Patch
** List<java.net.HttpCookie> allCookies() which returns a list of all
cookies
Again, we're limited here in what we can do for the first version. Also,
considering the modularity work for JDK 9
we are limited in the range of types we can reference from the "base"
module, where this API will reside.
* HttpHeaders: for the proposed first*() methods, please also consider
the signature: Optional<T> firstValueAsT(String name). This way, the
application developer can decide whether to use a default value or to
throw an exception (or anything else) when the header is absent.
How would that be implemented without knowing the type <T> ?
* HttpRequest/HttpResponse: why weren't the static methods added to
the interfaces
(HttpRequestBodyProcessor/HttpResponseBodyProcessor/MultiResponseProcessor)
instead? In my opinion, this would be more logical, and would simplify
the API of HttpRequest/HttpResponse.
I'll consider that.
* HttpRequest.Builder: add validation to header/method/setHeader
(according to the rules in RFC 7230) & throw an
IllegalArgumentException if validation fails
I was intending to only apply the generic parsing rules.
* HttpRequest.Builder: for "method", how is the parameter handled?
Since method is case-sensitive ( cf.
https://tools.ietf.org/html/rfc7230#section-3.1.1 ), creating a
request with "delete" may fail, simply because it should be "DELETE".
I think it would be good to at least add a Javadoc note about the
case-sensitivity, and that the standardized methods (
http://tools.ietf.org/html/rfc7231#section-4.1 ) are defined as
all-uppercase.
agreed.
* HttpRequest: I'd propose to rename fromByteArray(Iterator<byte[]>)
to fromByteArrays
agreed
* HttpRequest::fromString and HttpResponse::asString should take a
java.nio.charset.Charset as parameter
instead of the String name of the Charset? Will consider.
* HttpResponse: in my opinion, "asFile" should take a Path as
parameter & the parameter name should be "file", instead of "filename"
same as above
* HttpResponse: concerning the "asString" methods: they refer to "the
character set specified in the Content-encoding response header".
However, this should be "the character set specified in the charset
parameter of the Content-type response header".
Right. Good point.
* WebSocket: the documentation says "Once the WebSocket is available,
WebSocketMessages can be created and sent either blocking or
asynchronously.". What does it mean to create a message
asynchronously? And how can I send a message asynchronously (without
manually creating a CompletableFuture), as there isn't a "sendAsync"
method in WebSocket?
That's a mistake. The intent was that message sending would only be
blocking/synchronous. Whereas receiving could be either blocking
or asynchronous.
* WebSocket: in "Asynchronous example", I believe "sockets" should be
defined as "new CopyOnWriteArrayList<>()", since LinkedList is not
thread-safe? Also, I personally would declare "futures" as "new
ArrayList<>()" (unless there's a compelling reason to use LinkedList
of which I'm unaware?).
Yes, that was pointed out to me already. Thanks
* please consider adding a HttpResponseBodyProcessor implementation
"asDefined()", which uses the same mechanism as
java.net.URLConnection::getContent (i.e. using the Content-Type header
& ContentHandlers) to determine the object to return. This would allow
for easy migration from the old API to the new API. (the "defined" in
the method name refers to the fact that the value of the Content-Type
header is used)
How would you determine the type of object to return? I believe that was
one limitation of that API in URLConnection
* please consider adding a HttpResponseBodyProcessor implementation
"asFileDownload(Path downloadDirectory, OpenOption... openOptions)".
This would determine the filename automatically, exactly as browsers
do by inspecting e.g. the Content-Disposition header.
Interesting idea. Though there is nothing to prevent that being
implemented on top of this API
I won't respond to the documentation comments individually, but will
take them on board as appropriate.
Many thanks for your comprehensive review!
Michael
=== documentation ===
* as I'm sure you are aware, the package Javadoc should be updated to
document the new API. I also think it would be good to clarify its
relation to the old API, JAX-RS and Java API for WebSocket (the latter
2 of which will, I assume, in future versions use this API as the
basis for their Client API implementations).
* HttpClient: "Request builders are then created by calling
request(URI) if associated with an application created client."
rephrase as "Request builders that are associated with an
application-created client, are created by calling request(URI)."
* HttpClient.Builder: "If set, the first request to an origin server
using "http" URLs will attempt to upgrade to HTTP version 2. If that
fails, then following requests will not attempt to upgrade again."
** make "origin server" a hyperlink to
https://tools.ietf.org/html/rfc6454#section-4
** explicitly state "following requests to the same origin server"
* HttpRequest: "A request's uri, headers and body can be set."
change "uri" to "URI" & link it to java.net.URI
* HttpRequest: currently, all examples specify ".body(noBody())". This
gives the impression it's actually required. I propose to remove this
from all examples of GET requests (especially the one in the "Two
simple, example HTTP interactions" at the top).
* HttpRequest::fromString and HttpResponse::asString: replace
"ISO8859_1" with ISO-8859-1 and link to
java.nio.charset.StandardCharsets.ISO_8859_1
* HttpResponse: "such as String, byte arrays, Files."
change "Files" to "files", since "Files" suggests that it returns
java.io.File, when instead it's java.nio.file.Path
* HttpResponseBodyProcessor: "write responses to String, byte[], File,
Consumer<byte[]>"
change "File" to "file", since "File" suggests that it returns
java.io.File, when instead it's java.nio.file.Path
* HttpResponseBodyProcessor: "If an exact content length was provided
in onRequestStart(), then that number of bytes must be provided."
explicitly add "before returning null" at the end, so: "If an exact
content length was provided in onRequestStart(), then that number of
bytes must be provided before returning null."
* WebSocket: add a note that using a WebSocket in a try-with-resources
statement will cause the close to be done by closing the underlying
TCP connection & what the possible implications are.
=== spelling ===
* HttpHeaders: "read only"
should be "read-only"
* HttpRequest: "any type as body,"
either has missing text after the comma, or the comma should be a point
* HttpRequest: "client initiated"
should be "client-initiated"
* HttpRequest: "The response body can then be received (either
synchronous or asynchronously)"
should be "The response body can then be received (either
synchronously or asynchronously)."
* HttpRequest: "Path body = r2.body(asFile("/tmp/response.txt));"
should be "Path body = r2.body(asFile("/tmp/response.txt"));"
* HttpRequest: "All of above examples"
should be "All of the above examples"
* HttpRequest: "CompletableFuture<String>; last ="
should be "CompletableFuture<String> last ="
* HttpRequest: "Returns the HttpClient.requestHttp2() setting for this
request."
should note that this setting may have been overridden using
HttpRequest.Builder.requestHttp2(), and therefore is not always equal
to HttpClient.requestHttp2()
* HttpRequest.Builder: "A builder of HttpRequests"
should be "A builder of HttpRequests." (if you look at the package
overview, all classes except this one have a point at the end)
* HttpResponseBodyProcessor: "return null and the body"
has missing text after body
* MultiResponseProcessor: "provides the"
should be "Provides the"
* WebSocketMessage: "A WebSocketMessages (Binary or Text)"
should be "A WebSocketMessage (Binary or Text)"
On 9/03/2015 17:27, Michael McMahon wrote:
Hi,
JEP 110 HTTP 2 client
in JDK 9, is defining and implementing a new API for HTTP which also
supports
the new HTTP version 2 that has recently been working its way through
the IETF.
The work also includes support for websockets (RFC 6455).
In fact, the majority of the API is agnostic about the HTTP protocol
version, with only minor
configuration settings, and support for multiple responses (Http
server push) having any direct impact.
The HTTP API is defined around three main types (HttpClient, which is
the central
point for configuration of SSL, executor service cookie management
etc), HttpRequest
and HttpResponse (which should be self explanatory).
Requests are sent/received either synchronously (blocking) or in a
non-blocking (asynchronous)
mode using java.util.future.CompletableFuture which is a powerful new
framework for
asynchronous execution introduced in JDK 8.
The API docs can be seen at the link below:
http://cr.openjdk.java.net/~michaelm/httpclient/01/
All new classes and interfaces belong to the java.net package.
A prototype implementation of this API supporting HTTP/1.1 only, is
available and will
be uploaded into the JDK 9 sandbox forest in the coming day or two.
Comments welcome!
Thanks,
Michael.
