RFR: 8324868: debug agent does not properly handle interrupts of a virtual thread

[Serguei Spitsyn]

On Wed, 28 Feb 2024 05:24:49 GMT, Chris Plummer <cjplummer at 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