RFR: 8293696: java/nio/channels/DatagramChannel/SelectWhenRefused.java fails with "Unexpected wakeup" [v8]

[Daniel Fuchs]

On Mon, 7 Nov 2022 16:28:11 GMT, Darragh Clarke <duke at openjdk.org> wrote:

>> Added logging to SelectWhenRefused for an intermittent failure caused by unexpected wakeups, this includes trying to receive data if there is any available to identify where it is coming from.
>> 
>> I also set the test to run in othervm mode which should reduce the chances of this failure happening in the first place.
>
> Darragh Clarke has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - removed dupe line
>  - refactoring and cleanup

test/jdk/java/nio/channels/DatagramChannel/SelectWhenRefused.java line 60:

> 58:                 if (!testNoPUEBeforeConnection(dc, refuser, sel, i)) {
> 59:                     break;
> 60:                 }

Thanks for the refactoring. It does make the test much easier to read! But we can still go one bit further:
  1. we could have the method return true if the test was successful. That would make the method result easier to understand, and would avoid the negation in the if statement above, which would make it easier to read too!
  2. we could change the loop to run from `i = 1 ; i <= MAX_TRIES ; i++` - this way we could check whether the maximum number of attempts was reached within the loop by comparing `i == MAX_TRIES` without having to subtract 1.
  3. assuming the changes above, we should throw an AssertionError if i == MAX_TRIES and we haven't broken out of the loop yet. 
 
So that loop would turn into:

           for (int i = 1; i <= MAX_TRIES; i++) {
                if (testNoPUEBeforeConnection(dc, refuser, sel, i)) {
                    break; // test succeeded
                }
                assertNotEquals(i, MAX_TRIES, "testNoPUEBeforeConnection: too many retries");
           }


The two other loops could be modified in the same way.

test/jdk/java/nio/channels/DatagramChannel/SelectWhenRefused.java line 68:

> 66:                 for (int i = 0; i < MAX_TRIES; i++) {
> 67:                     if (!testPUEOnConnect(dc, refuser, sel, i)) {
> 68:                         break;

See suggestion above (1 to MAX_TRIES inclusive, assertNoEquals)

test/jdk/java/nio/channels/DatagramChannel/SelectWhenRefused.java line 79:

> 77:                 if (!testNoPUEAfterDisconnect(dc, refuser, sel, i)) {
> 78:                     break;
> 79:                 }

See suggestion above (1 to MAX_TRIES inclusive, assertNoEquals)

test/jdk/java/nio/channels/DatagramChannel/SelectWhenRefused.java line 115:

> 113:             // our expected refuser port, cannot run just exit.
> 114:             DatagramChannel.open().bind(refuser).close();
> 115:             throw new RuntimeException("Unexpected wakeup");

Assuming the changes suggested above, we could simplify that into:

    boolean mayRetry = checkUnexpectedWakeup(sel.selectedKeys());
    boolean tooManyRetries = retryCount >= MAX_TRIES;
    sel.selectedKeys().clear();
    if (mayRetry && !tooManyRetries) {
        return false; // will retry
    } 
    // BindException will be thrown if another service is using
    // our expected refuser port, cannot run just exit.
    DatagramChannel.open().bind(refuser).close();
    throw new RuntimeException("Unexpected wakeup"); 
}
return true; // test succeeded

test/jdk/java/nio/channels/DatagramChannel/SelectWhenRefused.java line 162:

> 160:             }
> 161:         }
> 162:         return false;

Assuming the changes suggested above that would become:

             boolean mayRetry = retryCount < MAX_TRIES;
             if (mayRetry) return false;
             throw new RuntimeException("PortUnreachableException not raised");
         } catch (PortUnreachableException pue) {
             System.out.println("Got expected PortUnreachableException " + pue);
         }
     }
     return true; // test succeeded

test/jdk/java/nio/channels/DatagramChannel/SelectWhenRefused.java line 190:

> 188:             throw new RuntimeException("Unexpected wakeup after disconnect");
> 189:         }
> 190:         return false;

Assuming the changes suggested above that would become:


    boolean mayRetry = checkUnexpectedWakeup(sel.selectedKeys());
    boolean tooManyRetries = retryCount >= MAX_TRIES;
    sel.selectedKeys().clear();
    if (mayRetry && !tooManyRetries) {
        return false; // will retry
    } 
    throw new RuntimeException("Unexpected wakeup after disconnect");
}
return true;  // test succeeded

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

PR: https://git.openjdk.org/jdk/pull/10851