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

[Patricio Chilano Mateo]

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

src/hotspot/share/runtime/objectMonitor.cpp line 1699:

> 1697:                                          : JavaThreadStatus::IN_OBJECT_WAIT;
> 1698:     java_lang_Thread::set_thread_status(current->threadObj(), state);
> 1699:   }

It does look a bit hacky. How about moving JavaThreadInObjectWaitState here and change the constructor instead to receive interruptible as argument. Something like:


JavaThreadInObjectWaitState(JavaThread *java_thread, bool interruptible, bool timed) :
    JavaThreadStatusChanger(java_thread, !interruptible ? java_lang_Thread::get_thread_status(java_thread->threadObj()) : timed ? JavaThreadStatus::IN_OBJECT_WAIT_TIMED : JavaThreadStatus::IN_OBJECT_WAIT) {

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23126#discussion_r1917203891