On Mon, 5 Jan 2026 13:09:48 GMT, Matthew Donovan <[email protected]> wrote:
>> Mikhail Yankelevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressing comments
>
> test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java
> line 47:
>
>> 45: * @library /test/lib
>> 46: *
>> 47: * @run main/othervm -Dtest.separateThreads=false CloseKeepAliveCached
>> false
>
> The `test.separateThreads` property is used to statically set
> `separateServerThread` on line 88, but then on line 230, you set
> `separateServerThread` based on `args[0]`. One of those could go away. I
> think each `@test` block should only need 1 `@run` instruction, right?
>
> Also, what is being tested differently if the server is in the "sub" thread
> versus when it is in the "main" thread?
Yes, you are right, we are duplicating the test. Will fix in the next commit
> test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java
> line 122:
>
>> 120: * Signal Client, we're ready for his connect.
>> 121: */
>> 122: serverReady = true;
>
> Change this to a `CountDownLatch`?
Done in the next commit
> test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java
> line 203:
>
>> 201: http.disconnect();
>> 202: } catch (IOException ioex) {
>> 203: if (sslServerSocket != null) {
>
> Is there a reason for closing the server socket in the client thread? If not,
> `sslServerSocket` could be made local to `doServerSide()`
This is only closing if error is encountered. Just in case. This seems to me to
be the easiest to read as well
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r3148666787
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r3148659683
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r3148658469
