[teststabilization] RFR: 8223638: Replace wildcard address with loopback or local host in tests - part 6

[Aleks Efimov]

Hi Daniel,

New webrev can be found here: 
http://cr.openjdk.java.net/~aefimov/8223638/02/

PerConnectionProxy.java - fixed as suggested.

MultiReleaseJarURLConnection.java:
I've added the toHttpJarURL method with small modifications:
Test now uses the method only for the case with "jar:http". For "http:" 
- it's been using only "http:" before the change. With "jar:" appended 
and without "!/" it fails with:

Caused by: java.lang.NullPointerException: no !/ in spec
	at java.base/sun.net.www.protocol.jar.Handler.parseAbsoluteSpec(Handler.java:168)
	at java.base/sun.net.www.protocol.jar.Handler.parseURL(Handler.java:150)
	at java.base/java.net.URL.<init>(URL.java:679)

Best Regards,
Aleksei

On 14/05/2019 10:18, Daniel Fuchs wrote:
> Hi Aleksei,
>
> Nit: PerConnectionProxy.java:
>
>  88                 InetSocketAddress isa = 
> InetSocketAddress.createUnresolved(InetAddress.getLoopbackAddress().getHostAddress(), 
> pserver.getPort());
>
> If you had named `serverAddress` `loopback` then you could trivially
> reuse it at the line above and reduce the line length.
> It's better to avoid too long lines as these are harder to review
> with frame diffs and side diffs.
>
> MultiReleaseJarURLConnection.java:
>
>  173                 {"http", 
> URIBuilder.newBuilder().scheme("jar:http").port(server.getPort()).loopback()
>  174 .path("/multi-release.jar!/").toURL()},
>  175                 {"http", 
> URIBuilder.newBuilder().scheme("http").port(server.getPort()).loopback()
>  176                         .path("/multi-release.jar").toURL()},
>
> There's too much going on on these lines. Can we rewrite this as:
>
>   {"http", toHttpJarURL(server.getPort(), "/multi-release.jar", "!/") },
>   {"http", toHttpJarURL(server.getPort(), "/multi-release.jar", "") },
>
> with
>
> private static URL toHttpJarURL(int port, String jar, String file)
>        throws MalformedURLException, URISyntaxException {
>     assert file.isEmpty() || file.startsWith("!");
>     URI httpURI = URIBuilder.newBuilder()
>                   .scheme("http")
>                   .loopback()
>                   .port(port)
>                   .path(jar);
>     return new URL("jar:" + httpURI + file);
> }
>
> best regards,
>
> -- daniel
>
> On 13/05/2019 19:45, Aleks Efimov wrote:
>> Hi Daniel,
>>
>> Thanks for all your comments.
>> I've changed all the tests to address the concern about the 
>> "localhost" configurations. Plus, I've utilized URIBuilder where 
>> possible.
>> Also tried keep the SimpleHttpServer simpler.
>>
>> The new version can be viewed here:
>> http://cr.openjdk.java.net/~aefimov/8223638/01/
>>
>> With Best Regards,
>> Aleksei
>>
>> On 13/05/2019 16:45, Daniel Fuchs wrote:
>>> Hi Aleksei,
>>>
>>> I believe that some configurations in the wild might
>>> return you the external host address when looking up
>>> "localhost". It doesn't matter if the server binds to
>>> the wildcard, but if you change the server to stop using
>>> the wildcard, then you also need to change the client to
>>> use the same address that the server binds too.
>>>
>>> AcceptCauseFileDescriptorLeak.java:
>>>
>>> I believe line
>>>   97                         (new Socket("localhost", 
>>> ss.getLocalPort())).close();
>>>
>>> should be changed to use the loopback address.
>>>
>>>
>>> UnreferencedSockets.java:
>>>
>>> same remark as above:
>>>
>>>  130         Socket s = new Socket("localhost", svr.localPort());
>>>
>>> PerConnectionProxy.java:
>>>
>>> same remark as above:
>>>
>>>   97                 InetSocketAddress isa = 
>>> InetSocketAddress.createUnresolved("localhost", pserver.getPort());
>>>
>>> Redirect307Test.java:
>>>
>>> should use URIBuilder here:
>>>
>>>  108         URL url = new URL("http://localhost:" + port);
>>>
>>> RedirectLimit.java:
>>>
>>>  113         URL url = new URL("http://localhost:" + port);
>>>
>>> should use URIBuilder.
>>>
>>> MultiReleaseJarHttpProperties.java:
>>>
>>>   69                 new URL("http://localhost:" + server.getPort() 
>>> + "/multi-release.jar")
>>>
>>> should use URIBuilder.
>>>
>>> MultiReleaseJarURLConnection.java:
>>>
>>>  171                 {"http", new URL("jar:http://localhost:" + 
>>> server.getPort() + "/multi-release.jar!/")},
>>>  172                 {"http", new URL("http://localhost:" + 
>>> server.getPort() + "/multi-release.jar")},
>>>
>>> Should use URIBuilder.
>>>
>>> SimpleHttpServer.java:
>>>
>>> a simpler change that keeps binding happening in
>>> start() would be:
>>>
>>>      private final HttpServer server;
>>> +    private final InetAddress address;
>>>
>>>      public SimpleHttpServer() throws IOException {
>>> -        server = HttpServer.create();
>>> +        this(null);
>>> +    }
>>> +
>>> +    public SimpleHttpServer(InetAddress address) throws IOException {
>>> +        address = addr;
>>> +        server = HttpServer.create();
>>>      }
>>>
>>>      public void start() throws IOException {
>>> -        server.bind(new InetSocketAddress(0), 0);
>>> +        server.bind(new InetSocketAddress(address, 0), 0);
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>
>>>
>>> On 13/05/2019 16:07, Aleks Efimov wrote:
>>>> Hi,
>>>>
>>>> Please help to review another part of test fixes to address 
>>>> intermittent  networking test failures.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~aefimov/8223638/00/index.html
>>>>
>>>> JBS:
>>>> https://bugs.openjdk.java.net/browse/JDK-8223638
>>>>
>>>> With Best Regards,
>>>> Aleksei
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20190514/e34f1d01/attachment.html>