On Tue, 21 Apr 2026 11:36: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. A few more test comments. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 44: > 42: public class SuspendWithObjectMonitorTimedWait extends DebugeeClass { > 43: > 44: static final Object startBarrier = new Object(); Using a `countDownLatch` is simpler than manual wait/notify. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 81: > 79: waitTask task = new waitTask(); > 80: Thread.Builder builder; > 81: builder = Thread.ofPlatform(); Suggestion: Thread.Builder builder = Thread.ofPlatform(); test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 175: > 173: status = DebugeeClass.TEST_FAILED; > 174: return status; > 175: } Is this really a failure? test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 179: > 177: if (failureCounter > 0) { > 178: status = DebugeeClass.TEST_FAILED; > 179: System.out.println("Grabbed the monitor in total " + > failureCounter + " times out of " + usefulRun + " useful runs, which is more > than 0."); You can just throw an exception on failure and do away with the status variable. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 206: > 204: throw new RuntimeException(e); > 205: } > 206: retryNumber+=1; Suggestion: retryNumber += 1; ------------- PR Review: https://git.openjdk.org/jdk/pull/30709#pullrequestreview-4153089961 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3122565822 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3122525568 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3122551841 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3122558290 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3122568717
