HTTP 2 client API

[Michael McMahon]

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.
>>
>