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.mcma...@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
