8354407: Test com/sun/management/OperatingSystemMXBean/GetProcessCpuLoad.java still fails on Windows

[kevinjwalls]

This is hard to reproduce, and at first I'd only seen -1 returned on the first calls to mbean.getProcessCpuLoad().
But eventually I observed a -1 at any time, including in middle of the iterations, or on the last iteration which makes the current test fail.

Should fail on Windows only if we only ever see -1 returned from getProcessCpuLoad().
Remove the "exclusiveAccess.dirs=." (JDK-8353231 adding "exclusiveAccess.dirs=." did not fix this.)

---

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-8354407: Test com/sun/management/OperatingSystemMXBean/GetProcessCpuLoad.java still fails on Windows (Bug - P4)

Reviewers

• Chris Plummer (@plummercj - Reviewer)
• Leonid Mesnik (@lmesnik - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24961

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

@kevinjwalls This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8354407: Test com/sun/management/OperatingSystemMXBean/GetProcessCpuLoad.java still fails on Windows

Reviewed-by: cjplummer, lmesnik

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 27 new commits pushed to the master branch:

• 2ad639f: 8356021: Use Double::hashCode in java.util.Locale::hashCode
• bee273d: 8297271: AccessFlag.maskToAccessFlags should be specific to class file version
• 34807df: 8355779: When no "signature_algorithms_cert" extension is present we do not apply certificate scope constraints to algorithms in "signature_algorithms" extension
• ... and 24 more: https://git.openjdk.org/jdk/compare/7d9a43839ad190a53efdbcc7afd1728760c14e21...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@kevinjwalls The following labels will be automatically applied to this pull request:

• jmx
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (d28b4c90)
• 00: Full (70cd88c3)

[plummercj]

The ex = null is a little misleading because actually it's irrelevant. An error on the next iteration will set it again, but it will be ignored because good != 0.

[kevinjwalls]

Yes, as long as there is a good value captured, Windows should pass.
We could get into how many good values we should see, I might suggest 6 out of 10? But that seems like a guessing game.
The breakage in this feature before meant it could never return a good value, that's what we need to guard against.

[lmesnik]

I would simplify to

 for (int i = 0; i < TEST_COUNT; i++) {
            double load = mbean.getProcessCpuLoad();
            if (load == -1.0 && Platform.isWindows()) {
                // Some Windows 2019 systems can return -1 for the first few reads.
                // Remember a -1 in case it never gets better.
                // Some Windows systems can return -1 occasionally, at any time.
                // Will fail if we never see good values.
               
            } else if (load < 0.0 || load > 1.0) {
                throw new RuntimeException("getProcessCpuLoad() returns " + load
                          + " which is not in the [0.0,1.0] interval");
            } else { 
               // we got at least one load from 0.0 to 1.0, that's good to pass on Wiindows
                good++;
          }
            try {
                Thread.sleep(200);

            }
        }

    
        if (good == 0 && Platform.isWindows()) {
            // Never get any good results on Windows 2019
            throw throw new RuntimeException("getProcessCpuLoad() returns  always -1.0 on Windows in 10 attempts. ");
        }
    }

does it makes a sense?

[kevinjwalls]

Thanks Leonid, yes we could do it like that. There have to be a load of ways we could arrange this. Yes, it could be a little simpler that what we have now, although it's not that complicated now...

[kevinjwalls]

I do like having the Exception created at the point where we recognise there's a problem. While the message could hard-code in the -1 in the Windows case, it must be also good to use the same expression to build the message, right next to each other.
I also like keeping the existing Exception wording, there isn't a real need to change it.
It's still a tiny little test routine and on future failure on Windows, it should be clear that we didn't receive any good values.
On balance would you be OK if we leave as it stands?

[lmesnik]

sure, let left it as it is.
Let return to this once 2019 is going to be EOL and remove all this stuff:
Looks good for me.

[kevinjwalls]

Thanks Leonid!

Eventually there were a few failures on other Windows versions, so I removed the specific mention of Windows 2019, although it was most common.

[plummercj]

The check for ex != null is not necessary. It should always be set if good == 0

[kevinjwalls]

Right, it does not really need to set ex = null as as soon as there is something in "good", we pass. But leaving the Exception hanging around seems wrong.

If good is zero then yes checking for ex is redundant. I left the check mostly to avoid any future change breaking it, but maybe it's best to expose that and make sure we fail if good is zero.

[plummercj]

Looks good.

[kevinjwalls]

Thanks for the reviews!

[kevinjwalls]

/integrate

[openjdk]

Going to push as commit 09cae5f.
Since your change was applied there have been 27 commits pushed to the master branch:

• 2ad639f: 8356021: Use Double::hashCode in java.util.Locale::hashCode
• bee273d: 8297271: AccessFlag.maskToAccessFlags should be specific to class file version
• 34807df: 8355779: When no "signature_algorithms_cert" extension is present we do not apply certificate scope constraints to algorithms in "signature_algorithms" extension
• ... and 24 more: https://git.openjdk.org/jdk/compare/7d9a43839ad190a53efdbcc7afd1728760c14e21...master

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinjwalls Pushed as commit 09cae5f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.