HTTP 2 client API

[Anthony Vanelverdinghe]

Hi Michael

Further feedback is in-line (please note that, to improve readability, I 
took the liberty to discard points without further feedback).

On 25/03/2015 18:33, Michael McMahon wrote:
>> * 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: [...]
After reading up on the different headers, I agree that 
Accept-Encoding/Content-Encoding must not be handled by the API. 
However, I believe that the TE/Transfer-Encoding headers should be fully 
supported, by  adding a "compression(boolean enable)" method to 
HttpClient.Builder and HttpRequest.Builder. This method would allow 
transparent compression by supporting at least one of the standardized 
algorithms (preferably gzip) [ 
http://www.iana.org/assignments/http-parameters/http-parameters.xhtml#transfer-coding 
].

Basically, I believe that the API should offer full support for the 
"low-level" RFC 7230 (Message Syntax and Routing), in which the 
"Transfer Codings" section specifies these headers. And since 
TE/Transfer-Encoding is the correct way for transparent on-the-fly 
compression, it seems logical to include support for it in the API (to 
me, it's on the same level as pipelining support, which is included in 
the API already).

>> * 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> ?
Sorry for the confusion, this wasn't meant to represent a generic 
method. I just meant to say: please consider returning an Optional 
instead of the current behavior of returning null or taking a "default 
value" parameter. So instead of the current methods:
String firstValue(String name)
long firstValueAsLong(String name, long defaultValue)

consider these methods instead:
Optional<String> firstValue(String name)
OptionalLong firstValueAsLong(String name)

The advantage of the latter methods would be that you can say for example:
headers.firstValue("Content-Type").orElse("application/octet-stream")
headers.firstValueAsLong("Content-Length").orElseThrow(() -> new 
IllegalStateException())

>> * HttpRequest::fromString and HttpResponse::asString should take a 
>> java.nio.charset.Charset as parameter
> instead of the String name of the Charset? Will consider.
I'd just like to point out that my main motivation for this is 
java.nio.charset.StandardCharsets. So for charsets such as UTF-8, UTF-16 
and US-ASCII, one can use a static import & simply write e.g. 
asString(UTF_8). Of course, if it's estimated that most of the uses of 
this method will use a non-standard charset, then it may be undesirable 
having to write asString(Charset.forName("Windows-1252")) instead.

On a sidenote, the method asString(String charset) in HttpResponse 
should specify to throw an UnsupportedCharsetException. And in the 
method fromString(String s, String charset) in HttpRequest, the 
parameter "s" should be renamed to "body".

>> * HttpResponse: in my opinion, "asFile" should take a Path as 
>> parameter & the parameter name should be "file", instead of "filename"
> same as above
An advantage of taking a Path, would be that you don't have to embed 
(OS-specific) file separators to construct the desired argument:
asFile(downloadDirectory.resolve(Paths.get("documents", "foo.pdf"));

>> * 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
This would be a HttpResponseBodyProcessor<Object>, and the type of 
object would just be whatever type the chosen ContentHandler returns 
(where the ContentHandler is chosen according to the algorithm specfied 
in java.net.ContentHandler).

After giving this some more thought, I think the proposed 
HttpResponseBodyProcessor isn't what I'm after. Actually, what I like 
about the old API, is that I can write ContentHandlers, specify the 
java.content.handler.pkgs property, and then, for example, automatically 
have urlConnection.getContent() return an instance of 
org.w3c.dom.Document for text/html documents.
To bring this idea to the new API, I would like to propose something 
like the following for consideration:

* use the java.util.ServiceLoader mechanism to find 
HttpResponseBodyProcessor implementations

* in HttpResponseBodyProcessor, add the methods:
boolean canProcess(HttpHeaders headers)
Class<T> getBodyClass()

* in HttpResponse, add the following method:
<T> T body(Class<T> bodyClass) {
     Iterator<HttpResponseBodyProcessor> processors = 
serviceLoader.iterator();
     Predicate filter = p -> p.canProcess(headers) && 
bodyClass.isAssignableFrom(p.getBodyClass());
     // convert the Iterator to a Stream
     HttpResponseBodyProcessor p = 
processorsStream.filter(filter).findFirst().orElseThrow(() -> new 
UnsupportedOperationException());
     return body(p);
}

So then I'd create a .jar with all my HttpResponseBodyProcessor 
implementations, and simply write:

HttpResponse response = client.request(new 
URI("http://www.foo.com/index.html")).get().send();
Document homePage = response.body(Document.class);

> Many thanks for your comprehensive review!
You're welcome,
Anthony

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