On Fri, 14 Nov 2025 12:15:35 GMT, Anton Artemov <[email protected]> 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
