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
