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

[David Holmes]

On Thu, 30 Jan 2025 12:15:28 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
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review: add an explaining comment; remove an extra empty line

I've suggested the comments I am looking for - thanks.

src/hotspot/share/services/threadService.hpp line 490:

> 488: 
> 489:  public:
> 490:   JavaThreadInObjectWaitState(JavaThread *java_thread, bool timed, bool interruptible) :

Suggestion:

  // Sets the java.lang.Thread state of the given JavaThread to reflect it is doing a regular, or timed, Object.wait call.
  //
  // The interruptible parameter, if false, indicates an internal uninterruptible wait, in which case we do not
  // update the java.lang.Thread state. We do that by passing the current state to the JavaThreadStatusChanger
  // so no actual change is observable, and skip the statistics updates. This avoids having to duplicate code
  // paths for the interruptible/ and non-interruptible cases in the caller.
  JavaThreadInObjectWaitState(JavaThread *java_thread, bool timed, bool interruptible) :

src/hotspot/share/services/threadService.hpp line 492:

> 490:   JavaThreadInObjectWaitState(JavaThread *java_thread, bool timed, bool interruptible) :
> 491:     JavaThreadStatusChanger(java_thread,
> 492:                             // This helper should do nothing when interruptible == false, so we set active = false.

What is `active`??

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23126#pullrequestreview-2585627714
PR Review Comment: https://git.openjdk.org/jdk/pull/23126#discussion_r1936654229
PR Review Comment: https://git.openjdk.org/jdk/pull/23126#discussion_r1936658727