On Wed, 28 Feb 2024 03:33:33 GMT, Serguei Spitsyn <sspit...@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. > > 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". ok > 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."` ? Ok. > 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. ok > 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. Using the interrupt is carried over from the original version of the test. I did consider replacing it, but opted to keep it in so there was one less change in the test to review. I can do as you suggested if you wish. I did have them sharing a common superclass at one point, but the only thing they ended up having in common is the `interruptee` field. They still each needed their own constructor and the `run()` methods. Although the `run()` methods are very similar looking, it is hard to factor them into common code due to the placement of the `synchronize` and the `wait()` call. I don't like the idea of a wait(5) that will always end out timing out. It's misleading. We aren't really waiting, we are sleeping, and would only have chosen wait() instead of sleep() to accommodate code sharing. It also would make the PreciseInterruptor have a wait() time of 5ms, which we don't want. In fact it can often take more than 5ms due to the single stepping that is going on at the same time. Keeping the code for each interruptor separate makes it clearer what each is attempting to do. > 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? `interruptsSent` is counted by the PreciseInterruptor and AggressiveInterruptor threads, which are in the same process, so we can print it here. When in remote mode, the counting is done by the RemoteInterruptor thread, which is in a different process, so we can't print it here. The RemoteInterruptor does print the count when it is done. > 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. The extra interrupt is because the interruptor will get another shot at interrupting once the interuptee does the following on the last interation (not this is done on every iteration of the main interruptee loop) synchronized(InterruptHangTarg.sync) { // Let the interruptor thread know it can start interrupting again sync.notify(); } Avoiding this last notify() would require that it be conditionally avoided on the last iteration of the loop (by comparing ii to INTERRUPTS_EXPECTED), and then as a result set some sort of flag so the PreciseInterruptor doesn't do another Thread.interrupt() after the interruptee wakes it up with the sync.notify(). Having a check in the middle of a loop to see if we are on the last iteration isn't exactly pretty either. I felt having the extra interrupt() was a better choice. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505388454 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505367553 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505388220 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505377699 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505380469 PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505388058
