RFR (10): 8182589 TLS SNI in new Java 9 client is not available

[Michael McMahon]

Third and hopefully final version at

http://cr.openjdk.java.net/~michaelm/8182589/webrev.3/index.html

- Michael

On 28/06/2017, 12:29, Michael McMahon wrote:
> Webrev updated at
>
> http://cr.openjdk.java.net/~michaelm/8182589/webrev.2/index.html
>
> Thanks
> Michael
>
> On 27/06/2017, 18:07, Michael McMahon wrote:
>> Yes, I agree Daniel. I will regenerate the webrev addressing that point.
>>
>> Thanks,
>>
>> Michael
>>
>> On 27/06/2017, 17:42, Daniel Fuchs wrote:
>>> On 27/06/2017 17:06, Michael McMahon wrote:
>>>> Thanks Daniel. The serverName comes from the InetSocketAddress for 
>>>> the destination.
>>>> It can't have a null field. Though I am wondering now if in the 
>>>> case of hardcoded IP address
>>>> destinations, that we shouldn't set the SNI hostname.
>>>
>>> In that case I wonder whether these two statements should be
>>> conditional to serverName != null?
>>>
>>>  140         SNIHostName sn = new SNIHostName(serverName);
>>>  141         sslParameters.setServerNames(List.of(sn));
>>>
>>> (IIRC there are 2 files)
>>>
>>> And if serverName can't be null - maybe a comment or an
>>> assert or an Objects.requireNonNull could make that clear.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>>
>>>> - Michael
>>>>
>>>> On 27/06/2017, 15:44, Daniel Fuchs wrote:
>>>>> Hi Michael,
>>>>>
>>>>> Looks good. I have imported your patch in my local
>>>>> workspace and played with it a bit.
>>>>>
>>>>> AsyncSSLDelegate.java:
>>>>>  128         this.serverName = sname;
>>>>> and SSLDelegate.java
>>>>>  63         this.serverName = sn;
>>>>>  70         serverName = sn;
>>>>>
>>>>> Should this fail if sname/sn is null? Or is null a valid value?
>>>>>
>>>>> HttpRequestImpl.java
>>>>>
>>>>>  61         this.method = builder.method() == null? "GET" : 
>>>>> builder.method();
>>>>>
>>>>> just a nit, but I think it might be better to use a local
>>>>> variable (here and at line 81) to avoid calling method()
>>>>> twice - and avoid having to wonder what will happens
>>>>> if the second call returns a different value.
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>> On 27/06/2017 12:17, Michael McMahon wrote:
>>>>>> Just to clarify, there are two issues in 8181422. This fixes one 
>>>>>> of them
>>>>>> but not the other. So, I will be leaving 8181422 open, and will 
>>>>>> just edit out
>>>>>> the part that is fixed (the NPE from not calling GET() )
>>>>>>
>>>>>> - Michael.
>>>>>>
>>>>>> On 27/06/2017, 10:26, Michael McMahon wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Could I get the following change reviewed please?
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~michaelm/8182589/webrev.1/
>>>>>>>
>>>>>>> This also fixes 8181422.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Michael
>>>>>
>>>