RFR: 8366659: ObjectMonitor::wait() can deadlock with a suspension request [v4]

Tue Nov 11 02:44:26 UTC 2025

On Mon, 10 Nov 2025 13:00:55 GMT, Anton Artemov <duke 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 deadlock is possible. There are two places where it can happen:
>> 
>> 1) 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.
>> 
>> 2) The race between suspension and retry: the thread could reacquire the OM and complete the wait() code in full, but then on return to Java it will be suspended while holding the OM.
>> 
>> 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. 
>> 
>> The JVMTI `waited` event posting (2nd one) is postponed until the suspended thread is resumed and has entered the OM again.  The `enter` to the OM (in case `ExitOnSuspend` did exit) is done without posting any events.
>> 
>> Tests are added for both scenarios. 
>> 
>> Tested in tiers 1 - 7.
>
> Anton Artemov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits:
> 
>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>  - 8366659: Added a comment to a boolean arg for enter()
>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>  - 8366659: Fixed new lines.
>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>  - 8366659: Removed incorrect assert,
>  - 8366659: Fixed merge conflict
>  - 8366659: Fixed whitespace.
>  - 8366659: Disabled posting JVMTI events in reenter-etner path of wait. Postponed waited event.
>  - ... and 8 more: https://git.openjdk.org/jdk/compare/79fee607...31482ba4

Vaious minor nits here with a couple of queries about why
assert_mark_word_consistency calls were removed.

During my crawl through I ran into quite a few nits and typos,
but I'm putting those in a separate issue.

Thanks for adding two new sub-test cases for better coverage.

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

> 531: }
> 532: 
> 533: void ObjectMonitor::enter_with_contention_mark(JavaThread* current, ObjectMonitorContentionMark &cm, bool post_jvmti_events) {

In the baseline also: `cm` is passed in, but it only used for this:
`assert(cm._monitor == this, "must be");`.

This makes me wonder if we're missing some code in `enter_with_contention_mark`
that is normally done when we are passed an `ObjectMonitorContentionMark`.

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

> 1106:   assert(currentNode->_thread == current, "invariant");
> 1107:   assert(_waiters > 0, "invariant");
> 1108:   assert_mark_word_consistency();

Why remove call to `assert_mark_word_consistency();`?

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

> 1184:   // Current has acquired the lock -- Unlink current from the _entry_list.
> 1185:   assert(has_owner(current), "invariant");
> 1186:   assert_mark_word_consistency();

Why remove call to `assert_mark_word_consistency();`?

test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 387:

> 385:                 }
> 386:                 try { Thread.sleep(1000);
> 387:                 } catch(Exception e) {}

Nit: The `Thread.sleep` call should be on its own line after L386.

test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 452:

> 450: 
> 451:             try { Thread.sleep(1000);
> 452:             } catch(Exception e) {}

Nit: The Thread.sleep call should be on its own line after L451.

test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 506:

> 504:                 }
> 505:                 try { Thread.sleep(1000);
> 506:                 } catch(Exception e) {}

Nit: The Thread.sleep call should be on its own line after L505.

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27040#pullrequestreview-3444783084
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2511785347
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2511693213
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2511698003
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2512616795
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2512620831
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2512622710