JDK 9 RFR of 8175209: Account for race condition in java/nio/channels/AsynchronousSocketChannel/Basic.java

[Brian Burkhalter]

Hi Hamlin,

Please see comments inline below.

On Mar 2, 2017, at 9:45 PM, Hamlin Li <huaming.li at oracle.com> wrote:
> I believe the patch will improve the stability of the test than original version.
> 
> But I'm afraid the patch will still face the intermittent failure, because there is still a time window for ch.write(genBuffer()); to run successfully, then exception is thrown at line 357.
> 
>  356                 ch.write(genBuffer());
>  357                 throw new RuntimeException("WritePendingException expected");
> 
> 
> It can be reproduced by putting below code in a loop as below, and reduce the sleep time to 10, my local test result is to fail at about one hundred or hundreds of loops.
> for (int i = 0; i < 1000; i++) { // put below code in a loop
> 
>  323             ch = AsynchronousSocketChannel.open();
>  324             ch.connect(server.address()).get();
> ......
>  345             int prevNumCompleted = numCompleted.get();
>  346             do {
>  347                 Thread.sleep(10);
>  348                 if (numCompleted.get() == prevNumCompleted) {
>  349                     break;
>  350                 }
>  351                 prevNumCompleted = numCompleted.get();
>  352             } while (true);
>  353 
> ......
>  369             if (!(writeException.get() instanceof AsynchronousCloseException))
>  370                 throw new RuntimeException("AsynchronousCloseException expected",
>  371                         writeException.get());
> } // end of for loop
> 
> The reason why it fails is because there are 2 writes,
>   write_1) one is at line 331 and line 334, they continuously write to buffer, the writes are expected to run successfully except of the time when ch is closed.
>   write_2) another is at line 356, it writes just once, and expected to fail with WritePendingException 
> write_1 and write_2 are racy, any time wait or sync would not change the racy relationship, so the intermittent failure is expected ( based on original code or your patch).
> 
> write_1)
>  331             ch.write(genBuffer(), ch, new CompletionHandler<Integer,AsynchronousSocketChannel>() {
>  332                 public void completed(Integer result, AsynchronousSocketChannel ch) {
>  333                     numCompleted.incrementAndGet();
>  334                     ch.write(genBuffer(), ch, this);
>  335                 }
> 
> write_2)
>  355             try {
>  356                 ch.write(genBuffer());
>  357                 throw new RuntimeException("WritePendingException expected");
>  358             } catch (WritePendingException x) {
I agree that there is still a race condition but I think it is much more likely to result in the expected exception than before, however it is still possible. In the loop you present above the sleep time is only 10 ms as opposed to the 1 second that I suggest in the patch so of course it is 100 times more probable that completed() will not have been invoked between pauses.

> I think we can use the racy relationship between write_1 and write_2 rather than try to avoid it, 
> How do you think about below solution?
>   make write_2 to write continuously to the channel too. As write_1 and write_2 are racy, WritePendingException is expected to happen either in write_1 or write_2. If either write_1 or write_2 throw WritePendingException, the test is passed; if no exception is thrown(in a reasonable time span or in infinite time?), then the test fails.

I actually thought of that as well. I think however that it is still possible that no WritePendingException is thrown if all the writes happen to “miss” each other. I don’t know that there is a way to make the exception 100% guaranteed. Your suggestion however might make it even more likely that the expected exception will be seen so I will investigate changing my patch.

Thanks,

Brian

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20170303/c08a4f1a/attachment.html>