On Fri, 20 Mar 2026 17:53:36 GMT, Mikhail Yankelevich
<[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).
>
> 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?
I have no objections to the additional configuration options. (i.e. leaving as
is.)
> 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");
There is no need to output the ssl debugging information by default, it just
adds lots of extra output that is usually never looked at. If there is a
problem with this test, developers can quickly toggle the switch to get the
needed debug info.
We just turned the default JSSE debug output off in many unneeded test cases.
([JDK-8368493](https://bugs.openjdk.org/browse/JDK-8368493)) No need to turn
it back on.
During the review of JDK-8368493, there was a suggestion to update all of the
JSSE test cases to use `test.debug`. We could consider that, but let's do that
as a unified chunk of work, rather than 1-offs here.
Thanks.
> 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?
Or in the finally block?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2968654637
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2968663961
PR Review Comment: https://git.openjdk.org/jdk/pull/30340#discussion_r2968651956
