RFR: 8087124 HTTP/2 implementation

[Chris Hegarty]

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
>