RFR: 8366659: ObjectMonitor::wait() liveness problem with a suspension request [v24]

[David Holmes]

On Mon, 19 Jan 2026 11:18:22 GMT, Anton Artemov <aartemov at openjdk.org> wrote:

>> Hi, please consider the following changes:
>> 
>> If suspension is allowed when a thread is re-entering an object monitor (OM), then a following liveness issues can happen in the `ObjectMonitor::wait()` method.
>> 
>> The waiting thread is made to be a successor and is unparked. Upon a suspension request, the thread will suspend itself whilst clearing the successor. The OM will be left unlocked (not grabbed by any thread), while the other threads are parked until a thread grabs the OM and the exits it. The suspended thread is on the entry-list and can be selected as a successor again. None of other threads can be woken up to grab the OM until the suspended thread has been resumed and successfully releases the OM.
>> 
>> This can happen in three places where the successor could be suspended: 
>> 
>> 1:
>> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1897
>> 
>> 2:
>> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1149
>> 
>> 3:
>> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1951
>> 
>> The issues are addressed by not allowing suspension in case 1, and by handling the suspension request at a later stage, after the thread has grabbed the OM in `reenter_internal()` in case 2. In case of a suspension request, the thread exits the OM and enters it again once resumed. 
>> 
>> Case 3 is handled by not transferring a thread to the `entry_list` in `notify_internal()` in case the corresponding JVMTI event is allowed. Instead, a tread is unparked and let run. Since it is not on the `entry_list`, it will not be chosen as a successor and it is no harm to suspend it if needed when posting the event. 
>> 
>> Possible issue of posting a `waited` event while still be suspended is addressed by adding a suspension check just before the posting of event.
>> 
>> Tests are added.
>> 
>> Tested in tiers 1 - 7.
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8366659: Addressed reviewers' comments, added comments, renamed tests.

Thanks for the updates. A few follow ups and queries.

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

> 2053:         // This is is now conditional as if the monitor_waited even
> 2054:         // is allowed, then a thread, even virtual, should not be moved to
> 2055:         // the entry_list, but rather unparked and let run. See the comment below.

Suggestion:

      // If we will add the vthread to the entry list below then we need to 
      // increment the counter *before* doing so.
      // Adding to _entry_list uses Atomic::cmpxchg() which already provides
      // a fence that prevents reordering of the stores.
      if (!JvmtiExport::should_post_monitor_waited()) {

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

> 2064:       // If the monitor_waited JVMTI event is not allowed, a thread is
> 2065:       // transferred to the entry_list, and it will eventually be unparked
> 2066:       // only when it is chosen to become a successor.

Suggestion:

      // only when it is chosen to become the successor.

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

> 2075:       // avoid a problem of having a suspension point when posting
> 2076:       // the monitor_waited JVMTI event, as suspending such a thread
> 2077:       // is no harm.

Suggestion:

      // However, if the monitor_waited event is allowed, then
      // the thread is set to state TS_RUN and unparked. The thread
      // will then contend directly to reacquire the monitor and
      // avoids being flagged as the successor. This avoids the problem
      // of having a thread suspend whilst it is the successor.

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

> 2101:         evt->unpark();
> 2102:       }
> 2103:       else if (java_lang_VirtualThread::set_onWaitingList(vthread, vthread_list_head())) {

Suggestion:

      } else if (java_lang_VirtualThread::set_onWaitingList(vthread, vthread_list_head())) {

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

> 2294:   // unparked directly in notify_internal(). Its state is then TS_RUN.
> 2295:   if (state == ObjectWaiter::TS_RUN) {
> 2296:     bool acquired = vthread_monitor_enter(current, node);

If we get here due to the direct unpark is it possible for `acquired` to be false? If so then I think the else clause starting at line 2312 below will be incorrect - it expects the thread to be on the entry list which it won't be.

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

PR Review: https://git.openjdk.org/jdk/pull/27040#pullrequestreview-3679921028
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706376692
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706377771
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706384082
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706385541
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706403647