On Wed, 28 Feb 2024 05:24:49 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> 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.
I'm okay to keep it as is, especially as the `interrupt()` was used before your
fixes.
I was thinking about something like below:
abstract static class Interruptor implements Runnable {
Thread interruptee;
boolean doStop = false;
abstract protected action();
void doStop() { doStop = true; }
public void run() {
synchronized (InterruptHangTarg.sync) {
while (!doStop) {
InterruptHangTarg.interruptsSent++;
interruptee.interrupt();
try {
action();
} catch (InterruptedException ee) {
System.out.println("Debuggee Interruptor: finished
after " +
InterruptHangTarg.interruptsSent + "
iterrupts");
break;
}
}
}
}
}
static class AggressiveInterruptor extends Interruptor {
AggressiveInterruptor(Thread interruptee) {
this.interruptee = interruptee;
}
protected action() {
InterruptHangTarg.sync.wait(5);
}
}
static class PreciseInterruptor extends Interruptor {
AggressiveInterruptor(Thread interruptee) {
this.interruptee = interruptee;
}
protected action() {
InterruptHangTarg.sync.wait();
}
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1505457981
