On Sat, 20 Apr 2024 23:13:48 GMT, Christoph Langer <[email protected]> wrote:
>> The test case sun/net/www/http/HttpClient/KeepAliveTest.java could be more
>> effective.
>>
>> It tests a matrix of HTTP client settings and server behavior, resulting in
>> 160 individual test scenarios. Each is tested in an own freshly spawned JVM
>> via the `@run main/othervm` directive. The need for new VMs is due to the
>> fact that the behavior of the HTTP client is determined at VM initialization
>> and can not be changed later on. However, for each distinct type of client
>> settings, one VM can be reused. This would lead us from 160 JVM
>> instantiations down to 16 which has a factor 10 influence on test runtime.
>>
>> E.g. on my developer laptop runtime went down from ~100s to ~10s.
>>
>> I also made additional cleanups/refactoring in the test.
>
> Christoph Langer has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains four additional
> commits since the last revision:
>
> - Simplify the test further
> - Merge branch 'master' into JDK-8330523
> - Small further cleanup
> - JDK-8330523
This looks reasonable.
test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 112:
> 110: private volatile boolean isProxySet;
> 111:
> 112: private CountDownLatch serverLatch = new CountDownLatch(1);
CountDownLatch is not reusable; consider using a Semaphore here
test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 113:
> 111:
> 112: private CountDownLatch serverLatch = new CountDownLatch(1);
> 113: private Thread server;
this can be converted to a local variable easily
test/jdk/sun/net/www/http/HttpClient/KeepAliveTest.java line 315:
> 313: keepAliveKeyClassconstructor.setAccessible(true);
> 314:
> 315: Logger logger =
> Logger.getLogger("sun.net.www.protocol.http.HttpURLConnection");
the logger was a static field for a reason; we don't want it to be GCed in the
middle of the test
-------------
PR Review: https://git.openjdk.org/jdk/pull/18817#pullrequestreview-2019394795
PR Review Comment: https://git.openjdk.org/jdk/pull/18817#discussion_r1577641489
PR Review Comment: https://git.openjdk.org/jdk/pull/18817#discussion_r1577643840
PR Review Comment: https://git.openjdk.org/jdk/pull/18817#discussion_r1577600648
