RFR: 8259628: jdk/net/ExtendedSocketOption/AsynchronousSocketChannelNAPITest.java fails intermittently

[Mark Sheppard]

On Wed, 20 Jan 2021 12:53:34 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Hi,
>> 
>> Could someone please review my fix for JDK-8259628: '`jdk/net/ExtendedSocketOption/AsynchronousSocketChannelNAPITest.java` fails intermittently' ?
>> 
>> `AsynchronousSocketChannelNAPITest` is failing intermittently on Linux due to a race condition caused by not correctly waiting for the result of an asynchronous operation. This fix rectifies this issue and adds additional checks to ensure correct result is received.
>> 
>> Kind regards,
>> Patrick
>
> LGTM. Thanks Patrick for taking this on!

This response maybe a bit of overkill or overthinking, but bear with me and apologies in advance!!
I think some of the additions have a few (theoretical) consequences - the read buffer and write buffer asserts cannot be assumed to be true as this, afaik, is not a datagram communication scenario ?? As such read and writes are not atomic.

Additionally they (may) distract from the main semantics of the test, that is, the read NAPI remain constant across reads.

The test has been modified to assert some conditions on the read and write buffer. These asserts, in theory, can not be assumed to hold true. Consider that the write returns a Future and will inform the writer how many bytes have been written. So writes are not atomic.
Additionally, as such,  a read may not actually read the number of bytes written by the writer, 
due to the underlying semantics of the supporting protocol (TCP/IP). If it was UDP as the underlying protocol then the read/write assertions would hold.
Because it is a co-located writer/reader test, and the size of the data transfer is small, the likelihood of any variation between the write and read is small.
So, do you really need to do the read/write buffer checks. Adding the get() method to the Future returned by the read should be sufficient to obviate the ReadPendingException

One other minor point: Is the assignment tempID = clientID;  needed for each iteration of the loop. The clientID should be constant across all reads.  If tempID was renamed originalClientID and assigned in the initialRun block

                     if (initialRun) {
                            assertTrue(clientID >= 0, "AsynchronousSocketChannel: Receiver");
                            initialRun = false;
                            originalClientID = clientID
                        } else {
                            assertEquals(clientID, originalClientID);
                        }

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

PR: https://git.openjdk.java.net/jdk/pull/2162