On Wed, 22 Apr 2026 11:17:30 GMT, Anton Artemov <[email protected]> wrote:
>> Hi, please consider the following changes: >> >> this is a fix for a bug in the `ObjectMonitor::wait()` method introduced by >> [8366659](https://bugs.openjdk.org/browse/JDK-8366659). This bug leads to >> increased failure rate of `ownedMonitorsAndFrames003`, though the test was >> very seldomly failing before >> [8366659](https://bugs.openjdk.org/browse/JDK-8366659). The fix just brings >> the failure rate back to where it used to be. >> >> In short, [8366659](https://bugs.openjdk.org/browse/JDK-8366659) introduced >> a code path where there was no handling of a suspension request until the >> thread goes back to Java, which caused test failures slightly more often. >> >> If `should_post_monitor_waited()` returns false, then a thread, which >> timed-out waiting and got a suspension request, does not have any suspension >> check. >> >> Now the extra condition is removed and a thread in the `TS_RUN` state will >> get its suspension request handled at the right place. >> >> A separate test which triggers the condition is added. >> >> Tested in tiers 1 - 7. >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Anton Artemov has updated the pull request incrementally with two additional > commits since the last revision: > > - 8382088: Addressed reviewer's comments. > - 8382088: Addressed reviewer's comments. Fix itself looks good. A few comments below, thanks. src/hotspot/share/runtime/objectMonitor.cpp line 1904: > 1902: // An event could have been enabled after notification, in this case > 1903: // a thread will have TS_ENTER state and posting the event may hit > a suspension point. > 1904: // From a debugging perspective, it is more important to have no > missing events. This comment was also added in JDK-8366659 but looks contradictory. If the monitor waited event is enabled after the thread was notified, then we don’t post the event since the thread will be in the `TS_ENTER` state. So it should say that it is more important to avoid deadlocks due to suspension. Also, this would not be a case of a missing event. What matters is if the event was enabled at the time of notification. Whether the waiter checks before or after the event is later enabled is racy and an implementation detail. Suggestion: Given that this suspension point is needed even without monitor waited event enabled, I would replace the comment with something like: “If the thread is not on the _entry_list, we need to check for suspension now because the re-enter path will not have a suspension point if there is no contention. Also, if the monitor waited event needs to be posted (which also implies off the _entry_list) we need to check for suspension before posting it.”. And we can also move `Post monitor waited event. Note that this is past-tense, we are done waiting.` below, where we check for the event. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 36: > 34: * @test > 35: * @bug 8382088 > 36: * @summary Suspend thread right after it timed out in wait() Suggestion: * @summary Check that a thread suspended at a timed wait() call does not reacquire the monitor before it is resumed. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 37: > 35: * @bug 8382088 > 36: * @summary Suspend thread right after it timed out in wait() > 37: * @requires vm.continuations Not needed. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 69: > 67: static long sleepInterval = 20; > 68: static int maxRetries = 2000; > 69: static long waitForTimedWaitingMills = 5000; These can be moved to the top where `lock` is defined. `sleepInterval` can be removed. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 80: > 78: waitTask task = new waitTask(countDownLatch); > 79: Thread.Builder builder = Thread.ofPlatform(); > 80: Thread targetThread = builder.name("Target > Thread").unstarted(task); Suggestion: Thread targetThread = Thread.ofPlatform().name("Target Thread").unstarted(task); test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 89: > 87: } > 88: > 89: Thread.yield(); Not needed. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 95: > 93: for (int n = 0; n < maxRetries; ++n) { > 94: > 95: long deadlineNanos = System.nanoTime() + > waitForTimedWaitingMills * 1000_000L; Suggestion: long deadlineNanos = System.nanoTime() + waitForTimedWaitingMills * 1_000_000L; test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 106: > 104: boolean grabbedMonitor = false; > 105: > 106: JVMTIUtils.suspendThread(targetThread); There is a race here where the target could have already exited before we try to suspend it. Adding a long `Thread.sleep` call before this statement will make the test fail. I see you have added several `isAlive()` checks in other places. All those can be removed and this race fixed just by letting the target synchronize with the main thread before exiting. You could use another `CountDownLatch` to fix it, but it will be simpler to just use one `Phaser`. You can use it to synchronize at the start of each loop iteration (simplify target to also just loop on `maxRetries`), and once more at the end before the target exits. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 124: > 122: usefulRun += 1; > 123: > 124: if (sleepInterval > 0) { This conditional is not needed. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 126: > 124: if (sleepInterval > 0) { > 125: try { > 126: Thread.sleep(sleepInterval); This can be `2 * timeout`. It will be clearer to the reader that we just want to sleep longer than what the target is waiting. ------------- Changes requested by pchilanomate (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/30709#pullrequestreview-4158246789 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127213607 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127215139 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127215533 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127216951 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127218218 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127218636 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127219477 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127221925 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127223153 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3127227247
