Hi Chris,
Comments below
On 27/04/2017, 14:32, Chris Hegarty wrote:
On 27 Apr 2017, at 10:18, Daniel Fuchs<daniel.fu...@oracle.com> wrote:
Hi Michael,
On 26/04/2017 16:22, Michael McMahon wrote:
Hi,
This webrev has been updated with a number of additional changes since
the first review.
The latest webrev is at:
http://cr.openjdk.java.net/~michaelm/8175814/webrev.3/index.html
1) AsyncDataReadQueue
I think you can ( should ) remove the type param from this class.
It appears to always deal with Http2Frame types.
I was originally trying to merge this class with Queue, which is generic.
It's something I would like to come back to later, but in the meantime
I can revert that change.
Is the endItem a settable thing? Seems that it is only ever one
possibility. L149 can either be removed or reinstated ( with the
knock on minor refactoring )
Again that was part of the effort to remove the dependency on Http2Frame.
But, I will come back to this later.
2) Stream
Remove the extra long line by adding a newline after the arrow:
private final static Supplier<Http2Frame> endItem = () ->
new DataFrame(0, DataFrame.END_STREAM, new ByteBufferReference[0]);
This code will be removed due to change above
3) AsyncSSLDelegate typo L264 don’t
ok
4) AsyncConnection / Queue
I find the term ‘block’ confusing here. It seems that the input channel,
in the AsyncSSLDelegate implicitly puts itself into “blocking” mode
when performing the initial handshake. The unblocking happens then
after successful negotiation of ALPN. Maybe some term to indicate
no higher-level callback? OR something else.
As far as Queue is concerned, it either works asynchronously or blocking.
If asynchronous then data is passed on through the callback. If blocking
then data has to be obtained by calling the blocking take() call.
Within AsyncSSLDelegate, the handshake is then done in blocking mode
but when finished it switches to the async mode.
Does that make it any clearer?
5) Is the AsyncSSLConnection effectively dead once stopAsyncReading
is invoke on it? It can only be used to seed another SSLConnection?
If so, maybe a comment to indicate this.
Right. I'll add a comment about that.
Thanks
Michael
-Chris.
