Thanks Roger. Some good catches there.
I've added the suggested spec changes (NPE clarifications) to my list
that will be dealt with later.
On the RedirectFilter comment, the exception is only thrown
in the case where the Location header is not present. So,
there isn't any further information available to be logged.
- Michael
On 18/02/16 17:02, Roger Riggs wrote:
Hi Michael,
A few comments, nothing severe.
Several properties are used in the implementation; is it significant
that some are sun.net and others java.net?
For the new package can we get away from using the "sun." prefix?
Exchange.java: 89 in cancel(), exchImpl may be null since it is not
initialized by the constructor
Methods like responseAsyncImpl0 should be private unless they are
needed outside the class.
It might help maintainability over time as different maintainers need
to understand the scope
of use.
ExecutorWrapper:82 println/printStackTrace! Perhaps Log or the system
logger instead
Http1Request: 46 ; a comment about which buffer indices are used for
what might be useful
Http1Request:172: finished() is synchronized but the assignment = true
(line 166 and 232) is not.
The field 'finished' may be hidden by 'finished' the local: Line 255.
Unused final static fields: CRLF_SIZE, CRLF_BUFFER,
EMPTY_CHUNK_HEADER_SIZE.
Http1Response:109 'finished' field should be private. (and probably
others)
HttpClientBuilderImpl:53 spec for cookieManager says it should be
non-null; add Objects.requireNonNull...
Ditto sslContext, sslParameters, executor, followRedirects, etc.
HttpClientImpl: 167 obsolete comment about ManagedLocalsThread (and no
explicit call to super() to inhibit inherited thread locals; if that's
needed)
HttpRequest.Builder does not prohibit supplying null to its methods.
Is it intentional to be able to supply null and get/revert to the
default behavior.
If so, it should be specified. Or the opposite requiring non-null.
HttpRequestBuilderImpl.java would be affected if the later.
HttpRequestBuilderImpl: 96; should copy() be doing a deep copy of the
User Headers?
Otherwise, subsequent changes to either HttpRequestBuilder would
affect the other.
RedirectFilter: 85: Invalid redirection exception should include the
invalid value for debug.
That's it for now,
Roger
On 2/4/16 11:14 AM, Michael McMahon wrote:
Hi,
The following webrevs are for the initial implementation of JEP 110.
Most of it is in the jdk repository with some build configuration in
the top
level repo for putting this code in its own module (java.httpclient).
http://cr.openjdk.java.net/~michaelm/8087112/01/top/webrev/
http://cr.openjdk.java.net/~michaelm/8087112/01/jdk/webrev/
The HTTP/2 implementation will come later.
Thanks,
Michael.
