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

[Brian Burkhalter]

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

Lines 41-42: Fields are not final.
Lines 114-116: Lines are not removed.

I am okay with the hard coded port number in the IANA reserved port range.

In retrospect, I think it might be better to change the test to fail fast, i.e., not ignore exceptions, so as not to hide any problems that the exceptions might expose. This could however be done under a different, subsequent PR, your choice.

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

PR Comment: https://git.openjdk.org/jdk/pull/19312#issuecomment-2142805689