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

[Daniel Fuchs]

Hi Michael,

On 28/06/2017 17:48, Michael McMahon wrote:
> Third and hopefully final version at
> 
> http://cr.openjdk.java.net/~michaelm/8182589/webrev.3/index.html

This one looks good to me! :-)

best regards,

-- daniel

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