RFR: 8087124 HTTP/2 implementation

[Michael McMahon]

Hi Chris,

Thanks for the review. I've incorporated your comments.
I'll push the current version today. Below is a webrev
of the exact changes. I will come back to other comments made
later and will make more changes then.

http://cr.openjdk.java.net/~michaelm/8087124/webrev.3/

Thanks,
Michael.


On 27/04/16 11:41, Chris Hegarty wrote:
> On 22 Apr 2016, at 22:51, Michael McMahon<michael.x.mcmahon at oracle.com>  wrote:
>
>> Hi,
>>
>> An updated webrev is available at:
>>
>> http://cr.openjdk.java.net/~michaelm/8087124/webrev.2/
>>
>> The main change is the removal of the permanently allocated
>> two threads per TCP connection (for HTTP/2)
> This is good.
>
> Http2ClientImpl.java
>
>    - HttpClientImpl client can be final
>    - The two collection types, opening and connections, can be private.
>      Since you are using them for locking, it is nice to know that they
>      cannot be accessed from outside the class
>    - getConnectionFor looks odd, in that it looks like the intention is to
>      block for a connection if another thread is already opening. In this
>      case it will return null! Also wait() should be in a conditional loop
>      to cater for spurious wake ups.
>    - getServerSettings appears to be unused. Can it be removed.
>
> HttpHeadersImpl.java
>
>    - You could use a new TreeMap<>(String.CASE_INSENSITIVE_ORDER)
>      to avoid all the toLowerCase’s
>    - Optional.ofNullable(null) -> Optional.empty()
>    - You can remove getOrCreate(), and replace it with computeIfAbsent, e.g.
>         headers.computeIfAbsent(name, k -> new LinkedList<>()).add(value)
>
> HttpClientImpl.java
>
>    - Please provide a default thread factory that names the HTTP Client
>      threads and uses the 5-arity Thread constructor.
>    - SelectorManager should call super(null, null, "SelectorManager", 0, false);
>    - SelectorManager.close should be volatile, to support async close
>
> HttpRequestBuilderImpl.java
>
>    - timeval = 0, redundant initialization.
>
> HttpRequestImpl.java
>
>    - Please remove unnecessary @param, etc, from createPushRequest,
>      and remove long lines.
>
> MultiExchange.java
>    - sun.net.httpclient.redirects.retrylimit -> java.net.httpclient… ??
>      In fact, all property names begin with sun.net. *
>
> Exchange.java
>
>    - responseImpl0 should be private, since it should be called in a doPriv
>
> Http2Connection.java
>
>    - processFrame, remove unnecessary @param, etc…
>    - general comment, check line lengths
>
> Http2Frame.java
>
>    -  setLength() -> computeLength() ??
>
> SettingsFrame.java
>
>    - public STATIC FINAL int TYPE = 0x4;
>
> Queue.java
>
>    - All fields can be private, q can be final also ( helps reason about the code )
>    - L59/80, the return is unnecessary
>    - @param callback is unnecessary
>    - T res = q.removeFirst(); return res;  Can be simply return q.removeFirst();
>      same comment for poll
>
> I assume the WebSocket types should not be in the webrev, and will not
> be in the push.
>
> -Chris.
>
>
>> Now, all socket reads and writes are done by the selector thread
>> (writes also happen on other threads). Asynchronous writes use the
>> same write() calls as the blocking interface. A new type AsyncConnection
>> defines a callback interface for asynchronous reads (and error reporting).
>> This interface is implemented by PlainHttpConnection and a new AsyncSSLConnection
>> (and AsyncSSLDelegate which is a non-blocking version of SSLDelegate).
>>
>> Thanks
>> Michael
>>
>> On 06/04/16 15:08, Michael McMahon wrote:
>>> Hi,
>>>
>>> This is the webrev for the HTTP/2 part of JEP 110.
>>>
>>> http://cr.openjdk.java.net/~michaelm/8087124/webrev.1/index.html
>>>
>>> There are minor changes to existing classes as well as the bulk
>>> of the new stuff in the new files. Most of the HTTP/2 implementation
>>> is in the files:
>>>
>>> src/java.httpclient/share/classes/java/net/http/Http2Connection.java
>>> src/java.httpclient/share/classes/java/net/http/Stream.java
>>>
>>> Each of the HTTP/2 frame types also has its own class derived from
>>> Http2Frame.
>>>
>>> The hpack code will be reviewed in a separate review to be
>>> circulated today by Pavel.
>>>
>>> I realise there were comments a few weeks ago on the Http/1 code
>>> which I haven't gotten back to. Once all of this code is integrated
>>> I will return to fixing up issues across the whole implementation,
>>> and some API issues will also be revisited.
>>>
>>> Thanks,
>>> Michael