RFR: 8230310: SocksSocketImpl should handle the IllegalArgumentException thrown by ProxySelector.select usage

[Daniel Fuchs]

Hi Jaikiran,

On 28/08/2019 15:47, Jaikiran Pai wrote:
> Can I please get a review and a sponsor for the patch for
> https://bugs.openjdk.java.net/browse/JDK-8230310? The patch is hosted as
> a webrev at [1].
> 
> As noted in that JBS issue, this is a follow-up patch to the changes
> that were done as part of [2]. During that patch review, it was decided
> that remaining usages of ProxySelector.select() would be updated
> accordingly if relevant. My search for this method usage shows this
> java.net.SocksSocketImpl as one such relevant place.

AFAICS your webrev looks fine, but could you try to write a test
for that?

> There are a couple of more usages in the main source code - one in
> jdk.internal.net.http.websocket.OpeningHandshake.proxyFor and the other
> in jdk.internal.net.http.HttpRequestImpl.retrieveProxy. Both these
> methods are private and the nature of their usage of
> ProxySelector.select suggests that they don't need any update and can
> continue to just throw the IAE as is. However, my knowledge of the
> codebase is very limited, so if these too need to be updated in some way
> (I don't think wrapping and throwing an IOException would be right),
> then do let me know, please.

I believe you are right. Both places checks the URI too - so anything
that would make ProxySelector::select fail will probably make
the checkURI() method throw IAE preemptively. I don't think
you would reach  ProxySelector::select if the URI had no authority,
for instance. That said - if ProxySelector::select throws IAE it
seems right to simply let it flow at those places.

> There are some usages in a tests, which I haven't yet looked much into.

Tests probably don't care - or they would have failed (hopefully).
Since the behavior of ProxySelector::select hasn't changed then what
you'd need to check is for tests that expect IAE when supplying a
bad URI - and I suspect the only tests that do that are the tests
you added.

best regards,

-- daniel

> 
> [1] http://cr.openjdk.java.net/~jpai/webrev/8230310/1/webrev/
> 
> [2] https://bugs.openjdk.java.net/browse/JDK-8177648
> 
> P.S: I created the JBS issue as a "Task". If it's supposed to be
> something else, please feel free to change it.
> 
> -Jaikiran
> 
>