RFR 8175814: HttpClient protocol version needs unspecified value

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR 8175814: HttpClient protocol version needs unspecified value

Michael McMahon
Hi

Could I get the following JDK 9 change reviewed, please?
In addition to fixing the spec problem around HTTP version,
it fixes an implementation issue with version also, where the per-request
version (if set) was not being picked up.

http://cr.openjdk.java.net/~michaelm/8175814/webrev.1/index.html

Thanks,

Michael
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Daniel Fuchs
On 01/03/17 15:40, Michael McMahon wrote:
> Hi
>
> Could I get the following JDK 9 change reviewed, please?
> In addition to fixing the spec problem around HTTP version,
> it fixes an implementation issue with version also, where the per-request
> version (if set) was not being picked up.
>
> http://cr.openjdk.java.net/~michaelm/8175814/webrev.1/index.html

Looks good to me Michael.

best regards,

-- daniel

>
> Thanks,
>
> Michael

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Chris Hegarty
On 06/03/17 11:00, Daniel Fuchs wrote:

> On 01/03/17 15:40, Michael McMahon wrote:
>> Hi
>>
>> Could I get the following JDK 9 change reviewed, please?
>> In addition to fixing the spec problem around HTTP version,
>> it fixes an implementation issue with version also, where the per-request
>> version (if set) was not being picked up.
>>
>> http://cr.openjdk.java.net/~michaelm/8175814/webrev.1/index.html
>
> Looks good to me Michael.

+1. I am happy to see the default version changed to HTTP/2.

Could a test for the default spec'ed HTTP/2 version be added.
We typically line up the module names, one per line, in the at
modules tag.

-Chris.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Michael McMahon


On 06/03/2017, 11:12, Chris Hegarty wrote:

> On 06/03/17 11:00, Daniel Fuchs wrote:
>> On 01/03/17 15:40, Michael McMahon wrote:
>>> Hi
>>>
>>> Could I get the following JDK 9 change reviewed, please?
>>> In addition to fixing the spec problem around HTTP version,
>>> it fixes an implementation issue with version also, where the
>>> per-request
>>> version (if set) was not being picked up.
>>>
>>> http://cr.openjdk.java.net/~michaelm/8175814/webrev.1/index.html
>>
>> Looks good to me Michael.
>
> +1. I am happy to see the default version changed to HTTP/2.
>
> Could a test for the default spec'ed HTTP/2 version be added.
> We typically line up the module names, one per line, in the at
> modules tag.
>
Yes, I'll add a test.

Thanks,
Michael.
> -Chris.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Michael McMahon
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

Thanks
Michael


On 06/03/2017, 11:29, Michael McMahon wrote:

>
>
> On 06/03/2017, 11:12, Chris Hegarty wrote:
>> On 06/03/17 11:00, Daniel Fuchs wrote:
>>> On 01/03/17 15:40, Michael McMahon wrote:
>>>> Hi
>>>>
>>>> Could I get the following JDK 9 change reviewed, please?
>>>> In addition to fixing the spec problem around HTTP version,
>>>> it fixes an implementation issue with version also, where the
>>>> per-request
>>>> version (if set) was not being picked up.
>>>>
>>>> http://cr.openjdk.java.net/~michaelm/8175814/webrev.1/index.html
>>>
>>> Looks good to me Michael.
>>
>> +1. I am happy to see the default version changed to HTTP/2.
>>
>> Could a test for the default spec'ed HTTP/2 version be added.
>> We typically line up the module names, one per line, in the at
>> modules tag.
>>
> Yes, I'll add a test.
>
> Thanks,
> Michael.
>> -Chris.
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Daniel Fuchs
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

The updates look good to me. Might be good to have another
set of eyes looking at it though.

best regards,

-- daniel

>
> Thanks
> Michael
>
>
> On 06/03/2017, 11:29, Michael McMahon wrote:
>>
>>
>> On 06/03/2017, 11:12, Chris Hegarty wrote:
>>> On 06/03/17 11:00, Daniel Fuchs wrote:
>>>> On 01/03/17 15:40, Michael McMahon wrote:
>>>>> Hi
>>>>>
>>>>> Could I get the following JDK 9 change reviewed, please?
>>>>> In addition to fixing the spec problem around HTTP version,
>>>>> it fixes an implementation issue with version also, where the
>>>>> per-request
>>>>> version (if set) was not being picked up.
>>>>>
>>>>> http://cr.openjdk.java.net/~michaelm/8175814/webrev.1/index.html
>>>>
>>>> Looks good to me Michael.
>>>
>>> +1. I am happy to see the default version changed to HTTP/2.
>>>
>>> Could a test for the default spec'ed HTTP/2 version be added.
>>> We typically line up the module names, one per line, in the at
>>> modules tag.
>>>
>> Yes, I'll add a test.
>>
>> Thanks,
>> Michael.
>>> -Chris.
>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Chris Hegarty

> On 27 Apr 2017, at 10:18, Daniel Fuchs <[hidden email]> 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.

  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 )

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]);

3) AsyncSSLDelegate typo L264  don’t

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.

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.

-Chris.
 
 
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Michael McMahon
Hi Chris,

Comments below

On 27/04/2017, 14:32, Chris Hegarty wrote:

>> On 27 Apr 2017, at 10:18, Daniel Fuchs<[hidden email]>  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.
>
>    
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Chris Hegarty

> On 27 Apr 2017, at 16:41, Michael McMahon <[hidden email]> wrote:
>
> ...
>> 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?

Thanks, it does. For my understanding …

The Queue type is basically an unbounded blocking queue. Adding items
will never block, since the queue can grow unbounded. Taking items will
block when the queue becomes empty. There are methods to poll, and to
push back items to the head. The queue also has a facility to register a
context-free trigger / callback when an item is added to the queue. The
callback has no knowledge of the queue or its internals, it just knows that
an item has been added.

How about block/unblock be renamed disable/enable [Put] Callback/Listener?

I’m less sure what, if anything, AsyncConnection.unblock could be renamed
to, since it has no knowledge of blocking or callbacks in the first place.

I’m just trying to move away from the overloading of “block” in this context.

-Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Michael McMahon
Hi Chris.

On 27/04/2017, 18:56, Chris Hegarty wrote:

>> On 27 Apr 2017, at 16:41, Michael McMahon<[hidden email]>  wrote:
>>
>> ...
>>> 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?
> Thanks, it does. For my understanding …
>
> The Queue type is basically an unbounded blocking queue. Adding items
> will never block, since the queue can grow unbounded. Taking items will
> block when the queue becomes empty. There are methods to poll, and to
> push back items to the head. The queue also has a facility to register a
> context-free trigger / callback when an item is added to the queue. The
> callback has no knowledge of the queue or its internals, it just knows that
> an item has been added.

Right. The producing side never blocks. The consuming side works either
through a callback or from a blocking take.
> How about block/unblock be renamed disable/enable [Put] Callback/Listener?

Ok, how about enable/disable Callback?

> I’m less sure what, if anything, AsyncConnection.unblock could be renamed
> to, since it has no knowledge of blocking or callbacks in the first place.
It would have to be enableCallback() from above. AsyncConnection does define
the callback API as well though.

Thanks,
Michael
> I’m just trying to move away from the overloading of “block” in this context.
>
> -Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8175814: HttpClient protocol version needs unspecified value

Chris Hegarty

On 28/04/17 11:24, Michael McMahon wrote:
> ...
> Ok, how about enable/disable Callback?
>
>> I’m less sure what, if anything, AsyncConnection.unblock could be renamed
>> to, since it has no knowledge of blocking or callbacks in the first
>> place.
> It would have to be enableCallback() from above. AsyncConnection does
> define
> the callback API as well though.

That works for me, thanks.

-Chris.
Loading...