On Fri, 12 Dec 2025 17:06:38 GMT, Mikhail Yankelevich
<[email protected]> wrote:
>> Arno Zeller 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:
>>
>> - Use CountDownLatch
>> - Merge remote-tracking branch 'upstream/master' into JDK-8371559
>> - Merge branch 'openjdk:master' into JDK-8371559
>> - Use timeout factor when waiting for server ready
>
> test/jdk/javax/net/ssl/Stapling/HttpsUrlConnClient.java line 36:
>
>> 34: */
>> 35:
>> 36: import java.io.*;
>
> minor: Since you're touching this file, could you please change wildcard
> imports to explicit
Done.
> test/jdk/javax/net/ssl/Stapling/HttpsUrlConnClient.java line 78:
>
>> 76: static String KEYALG;
>> 77:
>> 78: static final double TIMEOUT_FACTOR =
>> Double.parseDouble(System.getProperty("test.timeout.factor", "1.0"));
>
> Nit: could you please keep lines under 80 characters long?
Changed to use a CountDownLatch.
> test/jdk/javax/net/ssl/Stapling/HttpsUrlConnClient.java line 317:
>
>> 315:
>> 316: // Wait 5 seconds for server ready
>> 317: int maxWait = (int)(TIMEOUT_FACTOR * 100);
>
> Do you think it might be better to use something like
> `Duration.ofSeconds(Utils.adjustTimeout(20));` which will automatically scale
> sleep? You won't need to get the timeout factor either on the line 78
>
> Something like this.
> Suggestion:
>
> final Duration waitTime = Duration.ofSeconds(Utils.adjustTimeout(1));
> for (int i = 0; (i < 100 && !serverReady); i++) {
> Thread.sleep(waitTime);
> }
Changed to use a CountDownLatch.
> test/jdk/javax/net/ssl/Stapling/HttpsUrlConnClient.java line 322:
>
>> 320: }
>> 321: if (!serverReady) {
>> 322: System.out.println("Server wasn't ready after " + maxWait /
>> 20 + " seconds");
>
> I might be missing something, but why are you dividing by 20 here? Was it
> supposed to be 100 sec * timeout factor?
Changed to use a CountDownLatch.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28784#discussion_r2619323533
PR Review Comment: https://git.openjdk.org/jdk/pull/28784#discussion_r2619322589
PR Review Comment: https://git.openjdk.org/jdk/pull/28784#discussion_r2619322328
PR Review Comment: https://git.openjdk.org/jdk/pull/28784#discussion_r2619321932
