On Mon, 20 Apr 2026 11:50:00 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 one additional > commit since the last revision: > > 8382088: Fixed join timeout value. Functional fix is deceptively simple - I still need to think about it some more. Still getting my head around the test as well. src/hotspot/share/runtime/objectMonitor.cpp line 1905: > 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. > 1905: if (interruptible && node.TState != ObjectWaiter::TS_ENTER) { You need to update the comment at line 1911 as well as there is no re-check now. In fact you could just delete the comment on line 1911. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 35: > 33: * @test > 34: * @bug 8382088 > 35: * @summary Suspend thread right after it timed out in wait() Suggestion: * @summary Suspend thread right after it timed out in wait() test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 119: > 117: try { > 118: JVMTIUtils.suspendThread(targetThread); > 119: is_suspended = true; You can avoid the extra flag by doing: JVMTIUtils.suspendThread(targetThread); try { ... finally { JVMTIUtils.resumeThread(targetThread); } which is a more idiomatic try/finally block. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 174: > 172: } > 173: > 174: assertTrue(!targetThread.isAlive(), "target thread is still > alive"); You could do a timeless join and let the jtreg timeout deal with unexpected cases. That avoids the need to for the isAlive check. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 184: > 182: > 183: // Determined this purely experimentally > 184: if ((double)failureCounter / (double)usefulRun > > acceptableFailureRate) { These types of failure thresholds always tend to come back to bite us when tests run on a variety of machine and different loads, and different VM flags. ------------- PR Review: https://git.openjdk.org/jdk/pull/30709#pullrequestreview-4145330468 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3115305574 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3115307238 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3115341187 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3115358902 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3115353681
