RFR: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization

[Kevin Walls]

On Wed, 20 Sep 2023 22:08:27 GMT, Leonid Mesnik <lmesnik at 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.
>
> 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?

Thanks Leonid, I didn't see much value in that - we can see in the same file the return value of currentQueryIndexForProcess() is going to be -1 or an index from the for loop iteration from 0 to INT_MAX.
We can do that if you really feel it has a benefit?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15750#discussion_r1336871050