Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8259628: jdk/net/ExtendedSocketOption/AsynchronousSocketChannelNAPITest.java fails intermittently #2162

Closed
wants to merge 5 commits into from

Conversation

pconcannon
Copy link
Contributor

@pconcannon pconcannon commented Jan 20, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8259628: jdk/net/ExtendedSocketOption/AsynchronousSocketChannelNAPITest.java fails intermittently

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2162/head:pull/2162
$ git checkout pull/2162

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 20, 2021

👋 Welcome back pconcannon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 20, 2021
@openjdk
Copy link

openjdk bot commented Jan 20, 2021

@pconcannon The following label will be automatically applied to this pull request:

  • net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the net net-dev@openjdk.org label Jan 20, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 20, 2021

Webrevs

dfuch
dfuch approved these changes Jan 20, 2021
Copy link
Member

@dfuch dfuch left a comment

LGTM. Thanks Patrick for taking this on!

@openjdk
Copy link

openjdk bot commented Jan 20, 2021

@pconcannon This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 20, 2021
@msheppar
Copy link

msheppar commented Jan 20, 2021

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);
                    }

@dfuch
Copy link
Member

dfuch commented Jan 20, 2021

Hi Mark,

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.

My understanding is that read will read a full datagram (or nothing) and write will write a full datagram (or nothing). Indeed a datagram is the smallest unit that can be read or written on a UDP channel. Two calls to write() will produce two datagrams. Two calls to read() will attempt to read two datagrams.

best regards,

-- daniel

@dfuch
Copy link
Member

dfuch commented Jan 20, 2021

My understanding is that read will read a full datagram (or nothing) and write will write a full datagram (or nothing). Indeed a datagram is the smallest unit that can be read or written on a UDP channel. Two calls to write() will produce two datagrams. Two calls to read() will attempt to read two datagrams.

... but we don't have a datagram channel here: it's a regular socket channel ... sorry for the confusion.
Yes - objections accepted...

Copy link
Member

@dfuch dfuch left a comment

You could still do some checking if you wanted.
If you know that you have written nsent bytes, and that you later read nread bytes, then you could assert the following:

  1. nread <= nsent
  2. writeBuffer.mismatch(readBuffer) == (nread == nsent ? -1 : nread)

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jan 22, 2021
@msheppar
Copy link

msheppar commented Jan 22, 2021

Hi Daniel,
by adding a read/send bytes checks, are we not obscuring the purpose of the test and that's the NAPI check ?
Consider that, as the communication channel between the writer and sender is a stream, there are no (atomic) message boundaries, and in theory a read could be less than an individual write in one iteration, and then greater than the individual write in another iteration. Thus, adding an assert could give rise to unnecessary failures - it is unlikely in this test - in fact because of colocation you could place a strong bet that it won't happen.
Nonetheless, the following is possible
iteration N write n bytes, reads m where m < n (The underlying TCP protocol machine has sent m bytes and not the full n)
iteration N +1 write n bytes, read n + (n-m) i.e. the current write + the residue from the write in iteration N

and then an assert nread <= nsent will fail ?

In any case, the main thing is that the test is happy with its NAPI across the various reads

best regards
Mark

@dfuch
Copy link
Member

dfuch commented Jan 22, 2021

iteration N write n bytes, reads m where m < n (The underlying TCP protocol machine has sent m bytes and not the full n)
iteration N +1 write n bytes, read n + (n-m) i.e. the current write + the residue from the write in iteration N

and then an assert nread <= nsent will fail ?

Good point. You'd have to aggregate the results across multiple read writes and that would clutter the test.
OK - I withdraw my suggestion. What you have now is good enough Patrick.
Approved.

dfuch
dfuch approved these changes Jan 22, 2021
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 22, 2021
@pconcannon
Copy link
Contributor Author

pconcannon commented Jan 28, 2021

/integrate

@openjdk openjdk bot closed this Jan 28, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 28, 2021
@openjdk
Copy link

openjdk bot commented Jan 28, 2021

@pconcannon Pushed as commit 13ca433.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@pconcannon pconcannon deleted the JDK-8259628 branch Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
3 participants