On Wed, 22 Apr 2026 08:29:33 GMT, David Holmes <[email protected]> wrote:
>> 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.
>
> 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.
Yes, it looks like it, re-written the test with `countDownLatch`.
> 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();
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 175:
>
>> 173: status = DebugeeClass.TEST_FAILED;
>> 174: return status;
>> 175: }
>
> Is this really a failure?
Not really, but it is not a success either. With pass/fail binary logic
everything which is not a pass is a fail. So this should be a failure, as we
did not successfully tested what we intended to test.
> 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.
Done.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 206:
>
>> 204: throw new RuntimeException(e);
>> 205: }
>> 206: retryNumber+=1;
>
> Suggestion:
>
> retryNumber += 1;
Thanks for spotting, fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3123495270
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3123496021
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3123495717
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3123495478
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3123494936
