On Tue, 18 Mar 2025 02:27:31 GMT, Artur Barashev <[email protected]> wrote:
>> Fernando Guallini has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> SSLContextTemplate and using asserts
>
> test/jdk/sun/security/ssl/SSLSocketImpl/NonAutoClose.java line 65:
>
>> 63: */
>> 64: private final static boolean DEBUG =
>> Boolean.getBoolean("test.debug");
>> 65: private final static int NUM_ITERATIONS = 10;
>
> Why do we do 10 iterations of the same test?
By looking at the original issue, I believe the multiple iterations were added
to ensure that the synchronised behaviour is consistent over several cycles of
socket opening and closing, to test that there is no race condition when
waiting for the close_notify.
> test/jdk/sun/security/ssl/SSLSocketImpl/NonAutoClose.java line 135:
>
>> 133: */
>> 134: System.out.println("Waiting for server ready");
>> 135: if (!SERVER_READY.await(5, TimeUnit.SECONDS)) {
>
> Please see my other comment about `await()`.
Yes, that is fine, updated
> test/jdk/sun/security/ssl/SSLSocketImpl/SetClientMode.java line 100:
>
>> 98: connectedSocket.getSession();
>> 99:
>> 100: if (!HANDSHAKE_COMPLETE.await(5, TimeUnit.SECONDS)) {
>
> The test will be more robust if we do a simple `await()` call here. On some
> busy systems even 5 seconds may not be enough. JTEG tests have a default
> timeout of 2 minutes, so it won't wait forever in any case.
Yes, that is fine, updated
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23898#discussion_r2005462612
PR Review Comment: https://git.openjdk.org/jdk/pull/23898#discussion_r2005463263
PR Review Comment: https://git.openjdk.org/jdk/pull/23898#discussion_r2005464583
