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
