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
