RFR: 8087112 HTTP API and HTTP/1.1 implementation

[Roger Riggs]

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.