RFR: 8249824: s/n/w/p/https/HttpsURLConnection/CloseKeepAliveCached.java uses @ignore w/o bugid [v5]

[Volkan Yazici]

On Mon, 11 Aug 2025 15:45:17 GMT, Mikhail Yankelevich <myankelevich at openjdk.org> wrote:

>> * fully automated the test
>> * removed the race condition
>> * client on a thread and server on a thread options are now run together automatically
>
> Mikhail Yankelevich 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 five additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8249824
>  - cleanup and update of the code to the current style
>  - minor comment changes
>  - Apply suggestions from cr
>    
>    Co-authored-by: Daniel Fuchs <67001856+dfuch at users.noreply.github.com>
>  - JDK-8249824: s/n/w/p/https/HttpsURLConnection/CloseKeepAliveCached.java uses @ignore w/o bugid
>    
>    * fully automated the test
>    * removed the race condition
>    * client on a thread and server on a thread options are now run together automatically

test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java line 32:

> 30:  *
> 31:  * @run main/othervm -Dtest.separateThreads=true CloseKeepAliveCached false
> 32:  * @run main/othervm -Dtest.separateThreads=true CloseKeepAliveCached true

If `test.separateThreads` is always `true`, can we remove this property completely?

test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java line 106:

> 104:          */
> 105:         serverReady = true;
> 106:         SSLSocket sslSocket = (SSLSocket) sslServerSocket.accept();

One can say it is a matter of preference, but wrapping all `AutoCloseable`'s (`SSLServerSocket`, `SSLSocket`, `InputStream`, etc.) in a try-with-resources will help with ensuring all resources are correctly closed.

test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java line 120:

> 118:             String x;
> 119:             while ((x = r.readLine()) != null) {
> 120:                 if (x.isEmpty()) {

I guess this can be simplified to `while ((x = r.readLine()) != null && !x.isEmpty())`.

test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java line 135:

> 133:             out.flush();
> 134: 
> 135:             Thread.sleep(50);

Curious: Why do we need this?

test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java line 164:

> 162:             HttpsURLConnection.setDefaultHostnameVerifier(new NameVerifier());
> 163:             http = (HttpsURLConnection) url.openConnection();
> 164:             InputStream is = http.getInputStream();

See my comment above regarding `InputStream` and try-with-resources.

test/jdk/sun/net/www/protocol/https/HttpsURLConnection/CloseKeepAliveCached.java line 316:

> 314:     void startServer(boolean newThread) throws Exception {
> 315:         if (newThread) {
> 316:             serverThread = new Thread(() -> {

Nit: Unsolicited style changes can obstruct backporting and decrease version control hygiene. (This comment applies to changes starting from line 338 too.)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2269086136
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2269093354
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2269096196
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2269097406
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2269113381
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2269149826