On Fri, 23 Feb 2024 19:06:59 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
> Fix numerous debug agent issues and testing deficiencies related to > Thread.interrupt(). As a first read you should look at the description in the > CR. More details in first comment below. > > Also, this PR is dependent on the fix for > [JDK-8325187](https://bugs.openjdk.org/browse/JDK-8325187). The test might > generate GHA failures in the meantime. > > Tested with all svc tier1, tier2, and tier5 tests. The fix looks good to me. I've posted several nits/suggestions on the test update though. It is up to you to except my suggestions or not, so there is no pressure. test/jdk/com/sun/jdi/InterruptHangTest.java line 51: > 49: * Thread.interrupt() on the main thread, but does so in a > controlled > 50: * fashion that allows to test to verify that every interrupt is > 51: * received and handle. Nit: Typo? "handle" => "handled". test/jdk/com/sun/jdi/InterruptHangTest.java line 120: > 118: if (isInterrupted) { > 119: throw new RuntimeException("Thread is > interrupted but shouldn't be."); > 120: } Nit: This is confusing as we have just caught an expected `InterruptedException`. Should the message be instead: `"Thread is not expected to have the interrupted status."` ? test/jdk/com/sun/jdi/InterruptHangTest.java line 134: > 132: > 133: if (interruptorThread != null) { > 134: synchronized(InterruptHangTarg.sync) { Nit: A space is missed after `synchronized` keyword. test/jdk/com/sun/jdi/InterruptHangTest.java line 137: > 135: // Kill the interrupter thread > 136: interruptorThread.interrupt(); > 137: } Nit: Just a suggestion to consider replacing this `interruptorThread.interrupt()` with setting a status field, eg. `doStop`. It can be done in the same synchronized block: synchronized (InterruptHangTarg.sync) { interruptorThread.doStop(); } Both `AggressiveInterruptor` and `PreciseInterruptor` can subclass the `InterruptorThread`, so you can define the `interruptorThread` of type `InterruptorThread`. The class `InterruptorThread` can accumulate the common part of both interrupting threads. To make synchronization work correctly the `Thread.sleep(5)` in the `AggressiveInterruptor` can be replaced with `InterruptHangTarg.sync.wait(5)` similarly as it is done in the `PreciseInterruptor` but as timed-wait. The reason, I'm suggesting this is that using the `interrupt()` to interrupt `interruptedThread` adds a little confusion and not really necessary. This is in hope to make the code simpler. test/jdk/com/sun/jdi/InterruptHangTest.java line 140: > 138: } > 139: > 140: if (!remoteMode) { // we don't count interruptsSent when in > remote mode Nit: This is a little bit confusing. In fact the `interruptsSent` is counted at the line 215. The comment should probably say that we do not care (do not compare) this counter in the remote mode. Would it better to just print this counter unconditionally? test/jdk/com/sun/jdi/InterruptHangTest.java line 146: > 144: // When in precise mode, make sure that every interrupt sent > resulted in > 145: // an InterruptedException. Note the interruptor always ends up > sending > 146: // one extra interrupt at the end. Nit: Why do we need this extra interrupt. Can we re-organize the loop to get rid of this extra interrupt? It will make the logics a little bit more simple. ------------- PR Review: https://git.openjdk.org/jdk/pull/17989#pullrequestreview-1905242125 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505310083 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505311980 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505334486 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505346922 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505320368 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505322373
