Http client API

[Michael McMahon]

Mike. Thanks for the comments. I have commented on most of your points.
There are a few I'm not sure about, and maybe others will chime in.

- Michael.


On 08/08/2012 23:33, Mike Duigou wrote:
> General::
> - It's probably already been mentioned but having the classes in the httpclient package and most of them begin with "Http" seems redundant.
I think I'd agree if type names were always written as fully qualified 
names,
or if it were possible to import partial package names
eg. import java.net.* and then refer to httpclient.Request or 
httpclient.Response
but I don't think that is possible. Maybe we don't need a sub-package 
for httpclient
but it is a convenient grouping from a readability point of view.
What do other folks think?
> - Package JavaDoc is missing for both packages.
right we need to add that
> - Is the SPI package needed for just one class?
I suspect not. We probably should just bring it into the main package.
> - I am unsure if sendRequest should be a method of Client or Request.
>
That question arose before and there seems to be two differing preferences.
We probably should enumerate the arguments for and against the two.
> 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?
Yes, the provider needs work. I agree with the above. Though I don't 
know about the
necessity to provide multiple 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.
I'm not sure I follow the comment above. Does it just relate to 
implementation rather than the API?
> - HttpClient createClient() -- is this the same as HttpClientProvider.provider().createAsynchronousHttpClient()?
yes, and that needs to be documented.
> - 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.
Right. I agree (as others have suggested) a builder pattern for mutable 
state is a good idea. The current behavior of returning
the same client instance was more about simply a "fluent" programming style.
> - 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?
The idea behind connection cache was to allow applications more control 
over opening, closing and caching of TCP
connections. But, it's possibly not 100% right yet.  I figured that its 
scope would be at client level. If there were a higher
level notion of an application in Java SE, then it probably would be 
associated at that level imo.
In what way do you think it would be easy to mess things up?
> - setProxy lacks proxy authentication features. (sorry, I just said that to annoy you. :-) )
Yes. Authentication is one missing piece of the puzzle. We want to 
implement authentication
as a Filter, but it still needs a public API. But, it will be through 
that API where proxy authentication
happens also.
> - 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>)
Currently no. Since this is a new API, I'm not sure about the benefit of 
re-using the
legacy properties. Perhaps there could be value in allowing system proxy 
settings to
be used.
> - Is the default cookie manager from CookieHandler.getDefault() or is there no default?
Currently no default, which means no special treatment of cookie headers,
which I think is reasonable
> - Default is not specified for setAllowPipeLineMode() Presumably it is "false"
right. we need to specify default is "false"
> - There's no discussion of pipeline mode and connection cache.
ok
> - sendHeaders/sendRequest : presumably it is an error to use an HttpRequest from a different HttpClient.
that was one reason why the sendXXX methods made sense on HttpRequest, 
because otherwise
the possibility for sending a request on the wrong client has to be 
considered.
So, we will have to specify that if we keep it as it is.
> - setResponseBodyBufferLimit - like timeout this is a default. Perhaps setDefaultResponseBodyBufferLimit (long I know).
>
> - What happens with sendHeaders when the request already has a body?
right. should have an @exception IllegalStateException
> - Impact of closing the OutputStream returned from sendHeaders(). chunked mode vs. non-chucked.
There could be an IOException if writing or closing the stream 
in-appropriately. This should
be specified more explicitly in sendHeaders()
> - getResponse : java.util.concurrent.TimeoutException has a nice name but is an odd exception to be throwing. java.net.SocketTimeoutException seems more appropriate.
possibly, though the j.u.c.TimeoutException can be thrown when using a 
Future<HttpResponse>
So, the idea was to use the same exception class for all timeouts. It's 
an awkward choice though.
> - I'm curious why setUpgradeHandler() takes a class rather than an instance.
maybe Danny can comment on that.
> - 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().
good idea. It's always good to avoid nulls where possible.
> - I missed the "s" in setHttpsConfigurator until I looked at the method in more detail. It doesn't really stand out.
maybe setSSLConfigurator() and change HttpsConfigurator to SSLConfigurator?
> - getFilters() -- this is the only modify-in-place API. Kinda weird. I think having setFilters() would be better.
Do you mean getFilters() returns a copy and setFilters() would just set 
the actual list? Whatever
works best with the builder model, I suppose is what we should do.
>
>
> 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?
It was supposed to be usable directly by application code. So,
I don't think it could be package private.
>
>
> HttpRequest::
>
> - copy() vs clone()?
>
> - setFollowRedirects should get a default from HttpClient (or remove defaults for timeout and buffer limits). Asymmetry is annoying.
will probably add a default for follow redirects
> - 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.
Ok. So, a setBody(Iterator<ByteBuffer>)  then ?
> - setRequestURI() -- eliminate this unless there is a really good reason to keep it. Alternately, perhaps copy(URI) instead?
right. We need to clarify why that was put in there.
> - getMethod() -- missing.
ok
>
>
> HttpHeaders::
>
> - getValues() how about return empty result when there is no header of that name?
Right. The docs are inconsistent on that point. Empty list is what it 
should be
> - getValues() -- the returned list is unmodifiable, correct?
Right.
> - contains() -- IAE for null.
ok
> - getFirstValue() -- IAE for null name. It has to consistently be either IAE or NPE.
ok
> - getAllHeaderNames() -- unmodifiable list, correct?
right
>
>
> HttpResponse::
>
> - getBodyAsBytes(byte[], int) - I would rather the return the number of bytes put into the buffer.
yes. Actually the number of bytes read needs to be returned somewhere. 
It should return
an int or long
> - getBodyAsBytes(byte[], int) - Why would I ever want a max size less than the size of my byte buffer?
I originally wanted to implement the no-arg getBodyAsBytes() using the 
other method.
Maybe it's better to replace them both with:

long getBodyAsBytes(byte[] buffer) - buffer can't be null, use entire 
buffer, return # bytes read
byte[] getBodyAsBytes() - size limited by 
HttpClient.getResponseBodyBufferLimit()

> - getBodyAsBytes -- Document handling for content-length>  Integer.MAX_VALUE
dealt with implicitly from above change and the response body buffer 
limit itself.
> - getBodyAsByteBuffers -- handling for not enough space in the array needs to be documented. Unused entries nulled out?
situation would only arise through ByteBuffer allocation failure, and 
presumably OOM exception would be thrown.
Is it necessary to document this?
> - Perhaps maxlength ->  maxBytes to be more clear?
>
> - getBodyAsByteBufferSource -- if the same iterator is always returned why not return the iterator rather than an iterable?
The idea was to allow for use of the foreach convenience. eg

Iterable<ByteBuffer>  buffers = response.getBodyAsByteBufferSource();
for (ByteBuffer buffer : buffers) {
     // handle data in buffer
}


This raises the same concern then about the Iterable being 
non-restartable, which in this case
it clearly can't be. This is a "one-shot" streaming API. I notice that 
the documentation for
Iterable is rather terse and doesn't say whether
this usage is encouraged or discouraged. So, I'm not sure now. The 
question is if programmers would
really be confused by the fact that the Iterable can only be used to 
return one Iterator instance.
>
>
> 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.