On Wed, 28 Feb 2024 07:11:52 GMT, Serguei Spitsyn <[email protected]> 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
