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

[Michael McMahon]

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