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

[Chris Plummer]

On Wed, 28 Feb 2024 07:11:52 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> 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 void action();
> 
>         void doStop() { doStop = true; }
> 
>         public void run() {
>             synchronized (InterruptHangTarg.sync) {
>                 while (!doStop) {
>                     InterruptHangTarg.interruptsSent++;
>                     interruptee.interrupt();
>                     try {
>                         action();
>                     } catch (InterruptedException ee) {
>                         throw RuntimeException("Failed: InterruptedThread should not be interrupted!" + ee);
>                     }
>                 }
>             }
>         }
>     }
>     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();
>         }
>     }

The first version I did was just a single class that handled both interruptors, and looked similar to what you have above, except instead of a call to action() it instead had a conditional to decide what action to take based on the mode (precise or aggressvie).

The problem with this is the `sychronized` statement. Initially I thought it was benign for the aggressive case, and even had a comment saying it was unnecessary for the aggressive case, but doing so unconditionally made it easier to share code. However, I ran into some problem with it. I'm not exactly sure now what it was. I can see that the `synchronized` for interruptorThread.interrupt() call will end up deadlocking, although I don't recall that being the issue I had. For some reason I thought it was a problem at the start of the test, but I can't see what it might have been. Possibly due to other restructuring it no longer exists.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17989#discussion_r1506720062