On Mon, 10 Nov 2025 13:00:55 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. 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
