Hi Jaikiran,
On 29/07/2019, 13:12, Jaikiran Pai wrote:
Hello Michael,
Thank you for reviewing this. Comments inline.
On 24/07/19 10:37 PM, Michael McMahon wrote:
Hi Jaikiran,
Thanks for looking at this issue. ProxySelector::select is not
a particularly well specified method. The spec is a little vague
on what is expected of the supplied URI parameter.
It seems to me however, that the intent is that the supplied URI should
contain a valid protocol (scheme) and host, even though that is
perhaps not stated clearly enough. Though it does refer to
the protocol and the destination address as the components
it is looking for.
The concern I have with the suggested fix is that it appears
to contradict that implicit requirement.
I wonder if another solution is possible where the IAE is caught
at the appropriate place(s) and converted to an IOException
which is what users are expecting.
I can update the proposed patch to redo it to catch the IAE and throw
the IOException at the relevant places. Would you also want me to update
any javadoc of the ProxySelector to make it more clear of this (so far)
implicit behaviour? Does that require a CSR?
I think it would be good to do that, and it would require a CSR.
The existing @throws statement could be expanded to make this explicit
perhaps.
Thanks,
Michael
-Jaikiran
Michael.
On 19/07/2019, 16:14, Jaikiran Pai wrote:
Hello,
Could I please get a review and a sponsor for a patch which fixes an
issue in sun.net.spi.DefaultProxySelector? The patch is available as a
webrev at
http://cr.openjdk.java.net/~jpai/webrev/defaultproxyselector/1/webrev/
The issue has been reported in multiple different JBS issues[1][2][3]
all coming back to the same root cause. There probably are other similar
JBS issues related to this. The root cause is that the
DefaultProxySelector throws IllegalArgumentException when the passed URI
is non-null and thus contradicts its javadoc which (only) states the
IllegalArgumentException is thrown when the URI is null.
The change in the patch, now returns a Proxy.NO_PROXY when either the
protocol or the host of the passed URI is null. This is the same as
what's suggested in JDK-6797318.
In addition to the change to DefaultProxySelector, this patch also
changes sun.net.www.protocol.http.HttpURLConnection and
sun.net.www.protocol.ftp.FtpURLConnection. Just before invoking the
ProxySelector.select(...), both these classes use
sun.net.www.ParseUtil.toURI(...) which can return null. The changes to
these classes now ensure that the null isn't passed to the selector (to
avoid a IllegalArgumentException) and instead is handled properly.
2 new test classes have been added in the patch. These tests fail (as
expected) without the change to the source classes above and pass with
these changes. After these changes, locally, I have also run the
existing jtreg tests under test/jdk/java/net/HttpURLConnection and
test/jdk/sun/net/www/http/HttpURLConnection/ and they have passed
without regressions.
I haven't added anything new for the FTP testing, since I didn't find a
easy way to do that (based on my very limited search of existing tests).
[1] https://bugs.openjdk.java.net/browse/JDK-6797318
[2] https://bugs.openjdk.java.net/browse/JDK-8177648
[3] https://bugs.openjdk.java.net/browse/JDK-6563286
-Jaikiran
