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.
