On Tue, 23 Jul 2024 07:25:44 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The test
>> `serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java` is
>> failing with the assert in the `thaw_internal()` function. The assert is not
>> fully correct as it does not account for an unexpected scenario.
>>
>> Thanks to Patricio for reproducing this failure and identifying the root
>> cause:
>>> The problem is that we can unmount a virtual thread, then mount it again,
>>> thaw a few frames, execute code that acquires a JNI monitor, and then call
>>> thaw again without releasing that monitor. In this test this will happen if
>>> the vthread is unmounted in System.out.println("Thread doing JNI call: "
>>> ...) because of contention with the main thread doing
>>> System.out.println("Main waiting for event.").
>> The issue can be reproduced by adding Thread.yield() before
>> jniMonitorEnterAndLetObjectDie().
>>
>> The fix corrects the assert to account for the `thread->jni_monitor_count()`.
>> Question: Is the same scenario possible for non-JNI monitors as well?
>> Also, the fix includes the test tweak described above which makes this
>> failure always reproducible.
>>
>> Testing:
>> - Ran the test `GetOwnedMonitorInfoTest.java` locally
>> - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> comment tweak
Looks okay but one request.
Thanks
test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
line 89:
> 87: + Thread.currentThread().getName());
> 88:
> 89: Thread.yield();
Please add a comment explaining why the yield is present
-------------
PR Review: https://git.openjdk.org/jdk/pull/20294#pullrequestreview-2196517572
PR Review Comment: https://git.openjdk.org/jdk/pull/20294#discussion_r1689636214
