On Tue, 12 Aug 2025 08:22:48 GMT, Volkan Yazici <vyaz...@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+df...@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
