8281511: java/net/ipv6tests/UdpTest.java fails with checkTime failed

[serhiysachkov]

switching to nanoTime as suggested in ticket comments

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8281511: java/net/ipv6tests/UdpTest.java fails with checkTime failed (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23840/head:pull/23840
$ git checkout pull/23840

Update a local copy of the PR:
$ git checkout pull/23840
$ git pull https://git.openjdk.org/jdk.git pull/23840/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23840

View PR using the GUI difftool:
$ git pr show -t 23840

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23840.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back serhiysachkov! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@serhiysachkov 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.

[mlbridge]

Webrevs

• 03: Full - Incremental (27ef190d)
• 02: Full - Incremental (55547485)
• 01: Full - Incremental (a542d7b4)
• 00: Full (9173db16)

[dfuch]

I'd suggest to use TimeUnit to make times more readable and less prone to typing errors.

[msheppar]

the ubiquitous update of year in Copyright is required for the test file

[msheppar]

I think the checkTime from the base class Tests, should be reviewed in the context of the reported failures, where some of the actual timeouts registered are 10699 , 16926, 17926

The timeout check is

/* check the time got is within 50% of the time expected */
public static void checkTime (long got, long expected) {
checkTime(got, expected, expected);
}

/* check the time got is between start and end, given 50% tolerance */
public static void checkTime(long got, long start, long end) {
    dprintln("checkTime: got = " + got + " start = " + start + " end = " + end);
    long upper = end + (end / 2);
    long lower = start - (start / 2);
    if (got > upper || got < lower) {
        throw new RuntimeException("checkTime failed: got " + got
                + ", expected between " + start + " and " + end);
    }
}

which uses arbitrary upper bounds and lower bounds.

If a timeout is set at 5000 milliseconds then the expectation is that, should the timeout expire and a SocketTimeoutException is thrown, then the actual elapsed time will be equal to, or greater than, the specified timeout value passed to setSoTimeout. The minimum timeout is 5000 milliseconds, that is the lower bound for any timeout expiry check. Any earlier return would indicate a spurious wakeup by the OS.

In the checkTime a lower bound of less than the expected timeout is tolerated, and then an upper bound of an additional 50% is utilised as a check. Looking at the reported failures scenarios the actual elapsed time is significantly greater than the (timeout + 50%) value.

It is proffered that in reality no upper bound, other than > timeout value should be imposed on the timeout check.

[dfuch]

Additionally could you have a look at the superclass Tests.java? There is this code there:

    /* check the time got is within 50% of the time expected */
    public static void checkTime (long got, long expected) {
        checkTime(got, expected, expected);
    }

    /* check the time got is between start and end, given 50% tolerance */
    public static void checkTime(long got, long start, long end) {
        dprintln("checkTime: got = " + got + " start = " + start + " end = " + end);
        long upper = end + (end / 2);
        long lower = start - (start / 2);
        if (got > upper || got < lower) {
            throw new RuntimeException("checkTime failed: got " + got
                    + ", expected between " + start + " and " + end);
        }
    }

For the exception message, it would be better to use lower and upper instead of start and end - because when start == end the messsage lacks credibility.

[dfuch]

Actually, maybe it would be better to keep the expectedTime in milliseconds here, and convert System.nanoTime() - t1 to milliseconds before calling checkTime. We obviously don't need the nano second precision, and milliseconds are more human readable than nanoseconds when looking at the log.

[dfuch]

same remark here.

[dfuch]

I see my comment and @msheppar 's crossed each others. Whatever you decide to do here please fix the exception message to reflect the actual check that is being made.