Wow, Mike. Great feedback.
I just scanned your comments, and agree/understand most of them. I'll
help Michael to feed them back into the API. Though, there'll probably
be a few follow up mails to answers your direct questions and seek
further clarification before the new rev of the API.
Thanks again,
-Chris.
On 08/08/12 23:33, Mike Duigou wrote:
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.