RFR: 8366659: ObjectMonitor::wait() can deadlock with a suspension request [v8]
Daniel D. Daugherty
dcubed at openjdk.org
Fri Nov 14 18:19:40 UTC 2025
On Fri, 14 Nov 2025 12:15:35 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 deadlock is possible:
>>
>> 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 two 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
>>
>> 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 22 commits:
>
> - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
> - 8366659: Fixed the test.
> - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
> - 8366659: Fixed lines in tests.
> - 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
> - ... and 12 more: https://git.openjdk.org/jdk/compare/ff851de8...2ac18b94
Another crawl thru review of the test with more suggested changes.
I'll be doing one last round of commenting on the argument parsing stuff.
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 124:
> 122: // launch resumer enter threadLock
> 123: // <launch returns> while !READY_TO_NOTIFY resumer running
> 124: // delay 1-second threadLock.wait(1) wait for notify
We've deleted this sleep, so this line should change from:
// delay 1-second threadLock.wait(1) wait for notify
to:
// : threadLock.wait(1) wait for notify
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 128:
> 126: // set READY_TO_NOTIFY :
> 127: // threadLock.notify wait finishes :
> 128: // : reenter blocks :
We added a sleep here, so this line should change from:
// : reenter blocks :
to:
// : delay 200ms reenter blocks :
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 153:
> 151: // thread allows the waiter thread to loop tightly here:
> 152: // while !READY_TO_NOTIFY
> 153: // threadLock.wait(1)
Please replace this note with the following:
// Note: sleep(200ms) here while holding the threadLock to allow the waiter thread's
// timed wait to finish before we attempt to suspend the waiter thread.
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 212:
> 210: argsLeft--;
> 211: }
> 212: if (argsLeft == 0) {
Because of the addition of a mandatory argument (test case), argument parsing will
be more complicated. I need to mull on this a bit and I'll post another suggestion
after this round.
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 237:
> 235:
> 236: public static void usage() {
> 237: System.err.println("Usage: " + AGENT_LIB + " [N][-p][time_max]");
The `N` argument is not optional so it should not be surrounded by `[` and `]`.
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 239:
> 237: System.err.println("Usage: " + AGENT_LIB + " [N][-p][time_max]");
> 238: System.err.println("where:");
> 239: System.err.println(" N ::= test case");
s/case/case: 1 | 2 | 3/
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 470:
> 468: }
> 469: try {
> 470: Thread.sleep(1000);
Please add something like this above the `sleep`:
// Delay for 1-second while holding the threadLock to force the
// resumer thread to block on entering the threadLock.
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 552:
> 550: // wait for the waiter thread to block
> 551: logDebug("before contended enter wait");
> 552: int retCode = wait4ContendedEnter(waiter);
Since we are waiting here for ContendedEnter by the waiter thread, do we really
need the above `sleep(200)` or are we just being polite CPU resource wise?
test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 592:
> 590: }
> 591: try {
> 592: Thread.sleep(1000);
Please add something like this above the sleep:
// Delay for 1-second while holding the threadLock to force the
// resumer thread to block on entering the threadLock.
-------------
Marked as reviewed by dcubed (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/27040#pullrequestreview-3466061446
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528454607
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528465014
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528473772
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528423196
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528406725
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528408659
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528432284
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528443735
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2528446025
More information about the serviceability-dev
mailing list