RFR: 8346792: serviceability/jvmti/vthread/GetThreadState/GetThreadState.java testObjectWaitMillis failed

[David Holmes]

On Wed, 15 Jan 2025 08:15:40 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

> Below is the root cause of the bug described by Patricio:
> The root of the issue is similar to [JDK-8345543](https://bugs.openjdk.org/browse/JDK-8345543), but instead of `JavaThreadBlockedOnMonitorEnterState`, the interaction here is with its equivalent for Object.wait, `JavaThreadInObjectWaitState`.
> 
> We change the `Thread.State` of the carrier to `TIMED_WAITING` before we try preempting the `vthread`. So we can see the `Thread.State` of the vthread as `TIMED_WAITING` before even calling `try_preempt()` (when the state is `RUNNING` we return the `Thread.State` of the carrier). Then when we check for `JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER`, we caught the `vthread` in `afterYield()` still with the transitional state of `TIMED_WAITING`, which is mapped to `Thread.State.RUNNABLE`.
> 
> The fix is to delay the state change to `JavaThreadStatus::IN_OBJECT_WAIT*` the point after block with the `try_preempt()` call. The fix is not elegant and hacky but I do not see a better solution. The ugliness comes from the fact that the `ObjectMonitor::wait()` is called from two places: `ObjectSynchronizer::wait()` and `ObjectSynchronizer::waitUninterruptibly()`. We do not need the `JavaThreadInObjectWaitState` in the second cases, so the object is defined in the `JVM_MonitorWait()` but not in the `ObjectMonitor::wait()`.
> Maybe, deep refactoring the`JavaThreadInObjectWaitState` base class `JavaThreadStatusChanger` could be a better solution but it is going to be much more intrusive.
> 
> Testing:
>  - Checked the failed test does not fail with the fix: `serviceability/jvmti/vthread/GetThreadState/GetThreadState.java`
>  - Ran mach5 tiers 1-6

As with JDK-8345543 be very careful not to change the observable state/behaviour of regular threads to address a problem with virtual threads.

There are some quirks around the thread state changes and the various methods but they are generally intentional for regular threads. What we have, quirks and all, fits regular threads. In particular `wait_ininterruptibly`, as used by `ObjectLocker`, for class initialization is an implementation detail and the thread should NOT report that it doing an `Object.wait` call.

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

PR Review: https://git.openjdk.org/jdk/pull/23126#pullrequestreview-2554586000