RFR: 8247614: java/nio/channels/DatagramChannel/Connect.java timed out [v3]

[Alan Bateman]

On Wed, 21 Oct 2020 10:00:18 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> Occasional failures of this test have been observed. However it was unclear as to the precise nature of the failure due to minimal logging and multiple features of DatagramChannel being tested in one run. Another issue is that on failure of either a Writer or Reader thread, the thread that did not fail waits until the test itself times out. 
>> 
>> To attempt to mitigate these factors, the test was modified in the following manner:
>> 
>> - Additional logging was added to help locate future points of failure in the test.
>> - On L95, guards were added to make sure the test for a DatagramChannel throwing an AlreadyConnectedException does not end up using the same port as used for the connection on L86-87
>> - `wait(CompletableFuture<?>... futures)` was implemented to throw a CompletionException if either the Reader or Writer fails, rather than waiting for the test to time out.
>> 
>> Seeking review in particular on implementation of the `wait(CompletableFuture<?>... futures)` function. As it stands currently the wait function waits for one of the given futures completes exceptionally. If that doesnt happen, it will wait for _all_ futures to complete successfully.
>
> Conor Cleary has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8247614: Threadpool passed to invoke

No other comments, it's a good re-write of the test and the new version should be a lot more robust. I would be tempted to remove the author tag as the test is significantly replaced.

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

> 101:                 try {
> 102:                     InetSocketAddress otherAddress = new InetSocketAddress(address, (port == 3333 ? 3332 : 3333));
> 103:                     log.println("Testing if Actor throws already connected exception when attempting to send to " + otherAddress.toString());

Would you mind trimming down this very long line as it makes it too hard to do side-by-side diffs.

test/jdk/java/nio/channels/DatagramChannel/Connect.java line 82:

> 80:         final DatagramChannel dc;
> 81: 
> 82:         Actor(int port) throws IOException {

I think it would be clear to create Actor with a SocketAddress rather than a port. It didn't matter in the old test because the Reactor was bound to the wildcard address but here there is nothing to tell the Actor which address to connect it.

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

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