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

[Michael McMahon]

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