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

Wed Jan 21 05:05:56 UTC 2026

On Tue, 20 Jan 2026 09:56:09 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: Fixed whitespace.

Looks good overall, just a few comments. Thanks.

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

> 556:   bool is_virtual = ce != nullptr && ce->is_virtual_thread();
> 557:   if (is_virtual) {
> 558:     notify_contended_enter(current, post_jvmti_events);

We should also add `post_jvmti_event &&` to L567 below.

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

> 2063:     did_notify = true;
> 2064: 
> 2065:     if (!JvmtiExport::should_post_monitor_waited()) {

For the vthread case this value could be different from the one read above.

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

> 2077:       iterator->TState = ObjectWaiter::TS_RUN;
> 2078: 
> 2079:       OrderAccess::fence();

Is the fence needed? We already synchronize with `_wait_set_lock`.

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

> 2083:       if (!iterator->is_vthread()) {
> 2084:         iterator->wait_reenter_begin(this);
> 2085:         if (has_unmounted_vthreads()) {

The thread is going to call `enter` for this case so this is not needed. Same with `wait_reenter_begin` above.

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

> 2103:       }
> 2104:       return did_notify;
> 2105:     }

Given that there are more differences than common code, I think it's probably better to just separate the vthread and platform thread paths to make it easier to read. Something like this: https://github.com/openjdk/jdk/compare/pr/27040...pchilano:jdk:altnotify
You could add a comment on top of the method about the behavior when monitor_waited event is enabled.

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

> 2289:   // because, if monitor_waited JVMTI event is allowed, there can be a vthead which
> 2290:   // is not on the entry_list, but has been notified in the sense that it has been
> 2291:   // unparked directly in notify_internal(). Its state is then TS_RUN.

Suggestion for shorter comment:

// We check the state rather than was_notified because, when JVMTI
// monitor_waited event is enabled, the notifier only unparks the waiter
// without adding it to the entry_list.

src/hotspot/share/runtime/objectMonitor.hpp line 367:

> 365:   bool      enter_is_async_deflating();
> 366:   void      notify_contended_enter(JavaThread *current, bool post_jvmti_events = true);
> 367:   void      post_waited_event(JavaThread* current, EventJavaMonitorWait* wait_event, bool was_notified, ObjectWaiter* node, jlong millis, int ret);

Leftover?

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

PR Review: https://git.openjdk.org/jdk/pull/27040#pullrequestreview-3685329768
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710948914
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710964239
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710957840
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710958952
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710969804
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710956795
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710954338