On Wed, 20 Jan 2021 12:53:34 GMT, Daniel Fuchs <[email protected]> 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
