On Fri, 1 Mar 2024 23:26:48 GMT, Serguei Spitsyn <[email protected]> wrote:
> Please, review this fix correcting the JVMTI `RawMonitorWait()`
> implementation.
> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to update
> the interrupt status of the interrupted waiting thread. The issue is that
> when it calls `jt->is_interrupted(true)` to fetch and clear the `interrupt
> status` of the virtual thread, this information is not propagated to the
> `java.lang.Thread` instance.
> In the `VirtualThread` class implementation the `interrupt status` for
> virtual thread is stored for both virtual and carrier threads. It is because
> the implementation of object monitors for virtual threads is based on pinning
> virtual threads, and so, always operates on carrier thread. The fix is to
> clear the interrupt status for both virtual and carrier `java.lang.Thread`
> instances.
>
> Testing:
> - tested with new test
> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is
> passed with the fix and failed without it
> - ran mach5 tiers 1-6
Changes requested by cjplummer (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor/libInterruptRawMonitor.cpp
line 32:
> 30: static jvmtiEnv *jvmti = nullptr;
> 31:
> 32: static void check_thread_state(JNIEnv *jni, int check_idx) {
The name of this function could be better since it only checks that the state
is not interrupted. Maybe check_thread_not_interrupt().
test/hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor/libInterruptRawMonitor.cpp
line 36:
> 34:
> 35: LOG("check #%d: Thread State: (0x%x) %s\n", check_idx, state,
> TranslateState(state));
> 36: if (state & JVMTI_THREAD_STATE_INTERRUPTED) {
Should explicitly check `!= 0`
-------------
PR Review: https://git.openjdk.org/jdk/pull/18093#pullrequestreview-1915444251
PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1511855890
PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1511857152
