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

[Daniel Fuchs]

On Tue, 20 Oct 2020 11:40:23 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 two additional commits since the last revision:
> 
>  - 8247614: Actor/Reactor implement AutoCloseable
>  - 8247614: Thread pool created for use with futures

Good work altogether! One or two things to change yet and we'll get there :-)

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

> 62:         } finally {
> 63:             threadPool.shutdown();
> 64:         }

I believe it would be better to call threadPool.shutdown() **after** having closed the Actor/Reactor: closing the
Actor/Reactor makes sure that both tasks terminate - and shutdown() will wait for the tasks to terminate. Therefore I'd
suggest to create the threadPool in the `test` method instead, and pass it as first parameter to the `invoke` method.

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

> 127:
> 128:         @Override
> 129:         public void close() throws Exception {

`throws IOException` would be more appropriate here

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

Changes requested by dfuchs (Reviewer).

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