Http client API

[Mike Duigou]

Hi Michael!

I really look forward to using this API! It looks like you have made a lot of progress. Sorry for having so many comments on just one round.

Mike

General::
- It's probably already been mentioned but having the classes in the httpclient package and most of them begin with "Http" seems redundant.

- Package JavaDoc is missing for both packages.

- Is the SPI package needed for just one class?

- I am unsure if sendRequest should be a method of Client or Request.


HttpClientProvider::

- HttpClientProvider JavaDoc uses inconsistent capitalization of HTTP.

- createAsynchronousHttpClient() Since there's only one create method why bother to mention that it's "Asynchronous"?

- It's not clear how loadFromExternal() is different from provider().

- provider enumeration, request alternate providers?



HttpClient::

- There's no way to identify the source of the HttpClient(). ie. it is returned by provider() but there's no way to tell what you got. Would be useful for debugging to log to the file what provider you received.

- HttpClient createClient() -- is this the same as HttpClientProvider.provider().createAsynchronousHttpClient()?

- There doesn't seem to be a way to be truly thread safe about changes to configuration other than to set all configuration options before sharing an HttpClient instance and then never changing the configuration. Is changing the configuration *after* sharing realistic or should perhaps configuration be immutable (perhaps using a Builder pattern for HttpClient instances, ie. createClient() becomes clientBuilder()). Seeing that the set methods return HttpClient (the same instance unfortunately) it looks like you are considering or have considered using a builder style construction.

- Why expose the connection cache? Seems like just a good way to mess things up. Have you considered provider scope for connection cache? ie. all clients from the same provider share a cache?

- setProxy lacks proxy authentication features. (sorry, I just said that to annoy you. :-) )

- Does the proxy default to system properties and respect the nonProxyHosts? ie:
http.proxyHost (default: <none>)
http.proxyPort (default: 80 if http.proxyHost specified)
http.nonProxyHosts (default: <none>)

- Is the default cookie manager from CookieHandler.getDefault() or is there no default?

- Default is not specified for setAllowPipeLineMode() Presumably it is "false"

- There's no discussion of pipeline mode and connection cache.

- sendHeaders/sendRequest : presumably it is an error to use an HttpRequest from a different HttpClient.

- setResponseBodyBufferLimit - like timeout this is a default. Perhaps setDefaultResponseBodyBufferLimit (long I know).

- What happens with sendHeaders when the request already has a body?

- Impact of closing the OutputStream returned from sendHeaders(). chunked mode vs. non-chucked.

- getResponse : java.util.concurrent.TimeoutException has a nice name but is an odd exception to be throwing. java.net.SocketTimeoutException seems more appropriate.

- I'm curious why setUpgradeHandler() takes a class rather than an instance.

- getHttpsConfigurator() -- since a default is established before first invocation why not never return null. ie. implement the default behaviour in getHttpsConfigurator() and have the impl call getHttpsConfigurator().

- I missed the "s" in setHttpsConfigurator until I looked at the method in more detail. It doesn't really stand out.

- getFilters() -- this is the only modify-in-place API. Kinda weird. I think having setFilters() would be better. 



HttpUpgradeHandler::

- perhaps in the spi package?

- Method or methods to identify the UpgradeHandler. ie. getName(), getProtocols.



UpgradeConnection::

- Perhaps "Upgrade**d**Connection"? 

- There's no way to get back the UpdgradeHandler or source client.



SimpleConnectionClass:: 

- Package private?



HttpRequest::

- copy() vs clone()?

- setFollowRedirects should get a default from HttpClient (or remove defaults for timeout and buffer limits). Asymmetry is annoying.

- setBody(Iterable<ByteBuffer>, boolean isRestartable) -- This seems heinous. Non-restartable Iterables should be avoided. I like the suggestion of additional setBody(Iterable<ByteBuffer>) method instead.

- setRequestURI() -- eliminate this unless there is a really good reason to keep it. Alternately, perhaps copy(URI) instead?

- getMethod() -- missing.



HttpHeaders::

- getValues() how about return empty result when there is no header of that name?

- getValues() -- the returned list is unmodifiable, correct?

- contains() -- IAE for null.

- getFirstValue() -- IAE for null name. It has to consistently be either IAE or NPE.

- getAllHeaderNames() -- unmodifiable list, correct?



HttpResponse::

- getBodyAsBytes(byte[], int) - I would rather the return the number of bytes put into the buffer.

- getBodyAsBytes(byte[], int) - Why would I ever want a max size less than the size of my byte buffer?

- getBodyAsBytes -- Document handling for content-length > Integer.MAX_VALUE

- getBodyAsByteBuffers -- handling for not enough space in the array needs to be documented. Unused entries nulled out?

- Perhaps maxlength -> maxBytes to be more clear?

- getBodyAsByteBufferSource -- if the same iterator is always returned why not return the iterator rather than an iterable?



HttpConnectionCache.CachedConnection::
- I would expect this to be abstract and that client providers would extend.

- getCache() to return the client provider.


On Aug 7 2012, at 16:09 , Michael McMahon wrote:

> Hi,
> 
> A new revision of the Http client API planned for jdk 8 can be viewed
> at the following link
> 
> http://cr.openjdk.java.net/~michaelm/httpclient/v0.3/
> 
> We would like to review the api on this mailing list.
> So, all comments are welcome.
> 
> Thanks
> Michael McMahon.