Fwd: Http Client API for JDK 8

[Jim Gish]

-------- Original Message --------
> Subject: 	Http Client API for JDK 8
> Date: 	Thu, 03 May 2012 11:22:00 +0100
> From: 	Michael McMahon <michael.x.mcmahon at oracle.com>
> To: 	OpenJDK Network Dev list <net-dev at openjdk.java.net>
>
>
>
> Hi,
>
> Attached is a zipped bundle of a proposed new Http client API for JDK 8.
> The Javadoc can also be inspected at the following URI
>
> http://cr.openjdk.java.net/~michaelm/httpclient/v0.2/javadoc/
>
> The documentation is still somewhat sparse, but I hope there is enough
> to understand the intent behind all of the new classes/methods.
> It is still in an early stage of definition. So, all comments are welcome.
>
> Thanks,
> Michael
>
Hi Michael,

I have little expertise in this area, so I have only a few comments on 
content, but overall it looks good.  However, here are a few things that 
I noticed.

 1. An Overview would be helpful
 2. I think I understand the why you have capitalized Upgrade when
    referring to an "HTTP Upgrade" being done.  However, it doesn't
    strike me as necessary or particularly correct.  I would just use
    "upgrade".
 3. When viewing the http javadoc via the URL above, and horizontally
    expanding and shrinking the window,  the Method and Description
    content under Method Summary/Methods does not wrap and thus gets
    truncated - forcing the user to do horizontal scrolling to see the
    content.  This is also true for some, but not all, Method
    Detail(s).  For example, in httpclient.AsynchronousHttpClient, the
    method signatures for send and setAsynchronousChannelGroup do not
    wrap, although the descriptions for those methods do.  (This may be
    a consequence of doclet behavior rather than anything you are doing
    -- I don't know).
 4. HttpCompletionHandler
     1. "The HttpCompletionHandler is the completion handler implemented
        by applications that make use of the asynchronous mode in order
        to obtain notification when the HttpResponse is ready to be
        read." -- I would strike "the completion handler", yielding "The
        HttpCompletionHandler is the completion handler implemented by
        applications that make use of the asynchronous mode in order to
        obtain notification when the HttpResponse is ready to be read.
     2. "This method is always invoked once only for every request,
        regardless of whether an actual response was received from the
        serve." -> "This method is always invoked once only for every
        request, regardless of whether an actual response was received
        from the /*server*/. " (missing "r")
 5. Similarly to above, AsynchronousUpgradeConnection -- "The
    AsynchronousUpgradeConnection represents a connection obtained as
    the successful result of an HTTP Upgrade using the client in
    asynchronous mode."  -> "An AsynchronousUpgradeConnection is 
    obtained as the successful result of an HTTP Upgrade using the
    client in asynchronous mode."
 6. HttpConnectionCache - "The client calls before every request to
    either get an existing cached or newly opened connection.  When
    finisthed with a connection it calls" would be better as "The
    methods on this interface are called by the client prior to using or
    creating a connection and also to inform the cache that a connection
    is no longer needed by the client." (The second sentence as it is
    incomplete.)
     1. I think returnToCache should be called either releaseConnection
        or clear or freeConnection.  Actually, is this method really
        needed at all?  It seems like the close() on the connection
        should take care of this.  Either that or the method should be
        on the connection itself.
     2. "Represents a cache of idle TCP connections that can be managed
        according to a user (or application) defined policy"  -> should
        be "user-defined" or "application-defined", hence "user- (or
        application-)defined"  (Yeah I know.  This is a /real /nit! :-) )
 7. AsynchronousHttpClient -- "The type of client that sends an
    asynchronous request and receives the asynchronous response at some
    undetermined time later."  -> "A client that sends a request and
    asynchronously receives a response." (unless the server is pushing
    responses before getting a request, you typically would receive the
    response /later/ than the request :-)).  Also, I don't think you are
    /sending/ the request asynchronously, are you? Aren't  you sending a
    request to which you are requiring an asynchronous /response. /You
    certainly can describe this as an asynchronous request, but it's not
    quite correct to say that the request is being made asynchronously
    (unless of course it really is).  There is similar language in the
    send() methods' descriptions.
 8. AsynchronousByteChannel - getBody() "Returns an asynchronous byte
    channel to read read the response body." -> 'Returns an asynchronous
    byte channel to read read the response body." (2 occurrences -
    method summary and method detail)
 9. DataSource - "It can also be implemented by Filter modules for the
    purpose filtering all request or response body data." -> "It can
    also be implemented by Filter modules for the purpose /*of*
    /filtering all request or response body data. Note."
10. HttpMethodBase  - "MExtesible enum interface" /MExtesible/ ?
11. HttpUpgradeHandler
     1. "This interface is implemented by the client side of an Http
        Upgrade protocol handshake..." ->  "HTTP" or "http" instead of
        "Http"  for consistency throughout the doc. (Although, given the
        usage of camel case in the interface/class/method names, maybe
        not. In any case,  all three forms are used in various places in
        the docs)  Also, as before Upgrade -> upgrade.
     2. "If so, the Http Client library will request a reference to the
        protocol specific connection object by calling the getConnection
        method." -> /protocol-specific/
     3. "The HttpResponse obtained from an HttpRequest with an
        HttpUpgradeHandler carries the active connection
        (UpgradeConnection) object object created as a result of the
        handshake (if successful)." -> "The HttpResponse obtained from
        an HttpRequest with an HttpUpgradeHandler carries the active
        connection (UpgradeConnection) object object created as a result
        of a successful handshake."
12. UpgradeConnection
     1. "Interface to represent a connection managed in the
        HttpConnection Pool and which Encapsulates a SocketChannel"  -
        no need to capitalize "encapsulates."
     2. "The channel remains private and hidden. But all channels used
        by the client must be expose this API"  -> "The channel remains
        private and hidden, but all channels used by the client must
        expose this API." ("But. The"-> "but, the" and adding missing
        period after "API").
13. BlockingHttpClient -"The type of client that sends a request,
    blocking the current thread of execution until the response is ready
    to read."  ->  "A client that ....." (less wordy)
14. ByteBufferWrapper -implRestart()  "should be overridden if this
    class needs to do something when restart called Default is to do
    nothing."  no need to capitalize "default"
15. Filter - "The new request will sent and the response to that request
    will be linked back to the original request. ie the calling code
    will see the response to the new request as the response to the
    original request." -> "The new request will */be/* sent and the
    response to that request will be linked back to the original
    request*/, i.e./* the calling code will see the response to the new
    request as the response to the original request."
16. HttpClient
     1. "Each instance is responsible for a series of connections which
        all have the same characteristics (in particular execution
        mode), some of which can be overriden for a particular request,
        for example default request timeout."  -> "Each instance is
        responsible for a series of connections.  Each connection has
        the same characteristics, and in particular, the execution mode.
        Some of the characteristics, such as the default request
        timeout, can be overridden for a particular request."
     2. "Notes on Thread Safety For the blocking client, the API will be
        threadsafe." ->  "Notes on thread safety"  (should this be
        paragraph header with  "For the blocking client, the API will be
        threadsafe." the first sentence?
17. HttpConnectionCache.CachedConnection
     1. isProxy() - "returns if this connection is to a proxy," ->
        "returns */true/* if this connection is to a proxy,"
     2. getAge() - "returns age of TCP connection in milli-seconds at
        time of object creation." -> "returns /*the*/ age of TCP
        connection in milli-seconds at time of object creation."
     3. getUseCount() - "returns number of times the connection has been
        used" -> "returns /*the */number of times the connection has
        been used"
     4. isClosed() - "returns whether the connection is closed" ->
        "returns true iff the connection is closed"
     5. close() -- as I said before, perhaps this is all that is needed
        to "return" the connection to the cache.
18. HttpRequest - setBody() - "Sets this request's request body
    statically, ie. it is provided up-front. in the given ByteBuffer[]."
    -> "Sets this request's request body statically, /*i.e.*/ it is
    provided up-front. in the given ByteBuffer[]." (extraneous period
    between "up-front" and "in the given").
19. SimpleHttpConnectionCache - "Connections are stored in a HashMap." 
    Implementation details shouldn't be in the description.
20. HttpHeaderNames, HttpMethod -- aren't this elsewhere already?

Thanks,
    Jim



-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/net-dev/attachments/20120503/92915b4b/attachment.html