On Fri, 20 Mar 2026 16:23:45 GMT, Hai-May Chao <[email protected]> wrote:
> The SSLSocketSSLEngineCloseInbound.java test runs with different protocols,
> namely TLSv1.3, TLSv1.2, TLSv1.1, TLSv1, and TLS. The test log file shows
> that it completed successfully for the first four protocols, but it failed
> when running with protocol=TLS. Protocol TLS is resolved to TLSv1.3. The
> failure in the test is a timing-related issue, that server continues with
> write() after the client has already closed its side of the connection. This
> change removes the race condition by synchronizing the client and server with
> CountDownLatch.
>
> Have validated the change to ensure the test to pass (with
> JTREG=REPEAT_COUNT=50).
Some comments
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java
line 82:
> 80: * read() ... Finished
> 81: */
> 82: import javax.net.ssl.*;
nit: wildcard imports
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java
line 95:
> 93: * Enables logging of the SSL/TLS operations.
> 94: */
> 95: private static final boolean logging = true;
Since this logging is always set to true I personally don't think this should
be even included. What do you think about removing it and always logging?
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java
line 106:
> 104: * after gaining some familiarity with this application.
> 105: */
> 106: private static final boolean debug = false;
How abut making the ssl debug option as a part of `@run`? It is always false as
well which means that it does nothing atm.
If you believe it's needed, could you please change it so it takes a boolean
system property `test.debug`? Like this:
```java
Boolean.getBoolean("test.debug");
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java
line 230:
> 228: // server-side socket that will read
> 229: try (Socket socket = serverSocket.accept()) {
> 230: socket.setSoTimeout(500);
Suggestion:
socket.setSoTimeout((int)Utils.adjustTimeout(500));
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java
line 296:
> 294:
> 295: // Server signals client it has finished writing
> 296: serverReadyLatch.countDown();
minor: what if exceptions are thrown, would the test just time out? Wouldn't it
make sense to release the latch on the exceptions too?
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java
line 350:
> 348: // client-side socket
> 349: try (SSLSocket sslSocket =
> (SSLSocket)sslc.getSocketFactory().
> 350: createSocket("localhost", port)) {
Could you please change this to loopback address since you are touching the
file?
-------------
PR Review: https://git.openjdk.org/jdk/pull/30340#pullrequestreview-3983140991
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2967192534
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2967167995
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2967186052
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2967215782
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2967203151
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2967209499
