RFR: 8327650: Test java/nio/channels/DatagramChannel/StressNativeSignal.java timed out [v4]

[Alan Bateman]

On Tue, 28 May 2024 11:15:35 GMT, Mark Sheppard <msheppar at openjdk.org> wrote:

>> JDK-8327650 Test java/nio/channels/DatagramChannel/StressNativeSignal.java timed out 
>> 
>> Please oblige and review the following test only fix to alleviate a race condition within the test.
>> 
>> A race condition exists, in the test  StressNativeSignal, due to the setting of a condition variable, shouldTerminate, in the main thread’s invocation of UDPThread::terminate and the setting of shouldTerminate in the main run method of UDPThread.  A similar scenario exists for the ServerSocketThread.  The variable shouldTerminate is a member variable, and is by default set to false, so the setting to false in the run methods is not required. Removing shouldTerminate = false; statement mitigates the race condition.
>> 
>> Additionally, a number of other minor changes have been added to the test:
>> * it is desired to test the asynchronous close of the DatagramChannel::receive method which may trigger the NativeThread::signal issue, thus the while loop has been changed to a do { } while(); loop to invoke the DC receive method.
>> 
>> * The Thread::sleep has been replaced with a CountDownLatch to make the test slightly more deterministic.
>> 
>> It is noted in passing the ServerSocket::run method contains a redundant while loop for reading from an established connection stream, which is never created. This code is never reached as there is no peer to connect to the ServerSocket and the test is on the ServerSocket::accept method.
>> 
>> Another issue of note is that the test is using a hardcoded IANA assigned port 1122 (availant-mgr). But, this has not been altered. The ServerSocket test scenario could use an ephemeral port i.e. new ServerSocket(0);
>
> Mark Sheppard has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8327650: Test java/nio/channels/DatagramChannel/StressNativeSignal.java timed out — updated catch block to be consistent with idiom used in test

test/jdk/java/nio/channels/DatagramChannel/StressNativeSignal.java line 51:

> 49:             System.err.println("failed to create and start a ServerSocketThread");
> 50:             z.printStackTrace();
> 51:         }

It's not clear to me why there is a catch Exception here, same thing when creating/starting UDPThread, wouldn't it be simpler to have the constructor throw so that the test fails with the exception if there is an error starting either thread?

I assume the two fields can be final too.

test/jdk/java/nio/channels/DatagramChannel/StressNativeSignal.java line 103:

> 101:         public ServerSocketThread () throws Exception {
> 102: 
> 103:             socket = new ServerSocket(1122);

Re-existing issue but using a hardcoded port means it may fail if that port is already in used. I assume it can be changed to use the wildcard address.

test/jdk/java/nio/channels/DatagramChannel/StressNativeSignal.java line 113:

> 111:                 BufferedReader reader = new BufferedReader(new InputStreamReader(client.getInputStream()));
> 112:                 // the test never reaches here as the close will affect the blocking accept call
> 113:                 // and there are no clients to connect to this serversocket

Should this code be removed?

test/jdk/java/nio/channels/DatagramChannel/StressNativeSignal.java line 121:

> 119:                 z.printStackTrace();
> 120:                 if (!shouldTerminate) {
> 121:                     z.printStackTrace(System.err);

It looks like this will print the stack trace twice for the !shouldTerminate case.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19312#discussion_r1618880206
PR Review Comment: https://git.openjdk.org/jdk/pull/19312#discussion_r1618894833
PR Review Comment: https://git.openjdk.org/jdk/pull/19312#discussion_r1618894030
PR Review Comment: https://git.openjdk.org/jdk/pull/19312#discussion_r1618897097