On Mon, 11 Aug 2025 15:45:17 GMT, Mikhail Yankelevich
<myankelev...@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+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?
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
