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

[Mikhail Yankelevich]

On Tue, 12 Aug 2025 08:22:48 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> 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?

The initial manual test was testing both but manually. I have added another `@Test` with the `separeteThreads=false`. This way all combinations of client and server are covered as intended by initial test. 
Thank you for bringing my attention to this!

> 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.

Done in next commit

> 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())`.

I believe this is harder to read at a glance than the current expression, so I'd prefer to keep it

> 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?

just a leftover from my own testing, removed in the next commit. Thanks for noticing

> 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.

Done in next commit

> 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.)

Change was requested in the previous comments, so I believe this should be kept

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2276454772
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2276450917
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2276450602
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2276449200
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2276447648
PR Review Comment: https://git.openjdk.org/jdk/pull/23469#discussion_r2276447057