On Thu, 14 Sep 2023 17:24:39 GMT, Kevin Walls <kev...@openjdk.org> wrote:
> This assert happens rarely, but is seen in testing a few times. > > getCurrentQueryIndexForProcess comments that it can return -1, but it asserts > that the value is >=0 > > If we let it return -1 for failure as its comment documents, the caller can > handle the failure and not assert and end the JVM. > > Conversely, currentQueryIndexForProcess() clearly can return -1 on failure, > so add the comment like we already have in getCurrentQueryIndexForProcess(). > > This assert is not reproducing on demand, but with this change I've done 50+ > iterations of the test on windows-x64 and windows-x64-debug in mach5, and > hundreds locally. > > The test which has been seen to trigger the assert > "test/jdk/com/sun/management/OperatingSystemMXBean/GetProcessCpuLoad.java" > ...checks the range of the load value returned, and is happy enough if -1 is > the answer. Changes requested by lmesnik (Reviewer). src/jdk.management/windows/native/libmanagement_ext/OperatingSystemImpl.c line 780: > 778: int currentQueryIndex = currentQueryIndexForProcess(); > 779: > 780: assert(currentQueryIndex < numberOfJavaProcessesAtInitialization); doesn't it make sense to change assert to currentQueryIndex >= -1, so any other negative numbers are still hit assertion? ------------- PR Review: https://git.openjdk.org/jdk/pull/15750#pullrequestreview-1636577587 PR Review Comment: https://git.openjdk.org/jdk/pull/15750#discussion_r1332230115
