Re: Endless loop in EventDispatchThread - proposed solution

[Artem Ananiev]

Hi, Clemens,

thanks for digging into that. AWT shutdown code is far from perfect, but 
this is not a reason for not fixing bugs there :)

See my comments below.

On 8/21/2011 3:25 PM, Clemens Eisserer wrote:
Hi,

On our caciocavallo-web demo server I sometimes experience endless
spinning in an EventDispatchThread (in run()) which should have been
shut down long ago by AppContext.dispose() - causing our server to
consume 100% cpu.

I tried to anylze the situation using a remote debugger, this is what
I've found:

The thread is interrupted, therefor no events are dispatched anymore by
pumpEventsForFilter:

           while (doDispatch && cond.evaluate()) {
                 if (isInterrupted() || !pumpOneEventForFilters(id)) {
                     doDispatch = false;
                 }
             }


however, detaching the thread from the EventQueue fails, because there
are still events left for dispatching,
which in turn won't be dispatched, because the thread is interrupted.

1.) A fix would be to check for isInterruped also in the thread's run loop:

         public void run() {
             while (true) {
                 try {
                     pumpEvents(new Conditional() {
                         public boolean evaluate() {
                             return true;
                         }
                     });
                 } finally {
                     EventQueue eq = getEventQueue();
                     if (eq.detachDispatchThread(this) ||
    threadDeathCaught || isInterrupted()) {
                         break;
                     }
                 }
             }
         }


What do you think about the proposed solutions?

I may be a bit naive: to me, if a thread is interrupted, it should 
gracefully go away (and this is what AppContext.dispose() relies on), so 
in general I like the idea of handling isInterrupted() in 
EventDispatchThread.

I tried to slightly change your patch. The problem with checking for 
isInterrupted() in EDT.run(), as well as for threadDeathCaught, is that 
the thread will never be marked free in AWTAutoShutdown. To fix that, we 
should make EQ.detachDispatchThread() unconditional, so it always 
detaches the thread, but this would lead to the very next postEvent() 
will initialize a new dispatch thread...

2.) A big mystery for me was, why the thread hasn't been killed by
Thread.stop(), called by AppContext.dispose() - and why the
"threadDeathCaught"-variable was still false (as this would have caused
the thread to exit anyway).
The reason was, ThreadDeath is swollowed by the
IllegalMonitorStateException caused by Thread.stop():

Basically this is what is going on, when waiting for new events:

  pushPopLock.lock();
         try {
           cond.await(); //Throws ThreadDeath
         }finally {
            pushPopLock.unlock(); //Throws IllegalMonitorStateException,
"swollows" ThreadDeath
         }

So ThreadDeath is never catched.

This bug has been introduced when converting to java.util.concurrent, as
java monitors don't throw an exception in this situation.
My proposal for this bug would be to do away with the
threadDeathCaught-variable completly, as the isInterrupted()-check
proposed in 1.) will stop the thread anyway.

What if somebody calls EDT.stop() directly, without preceding 
interrupt()? I understand this is not a typical scenario, and we 
probably shouldn't support it, but we should at least not hang.

Thanks, Clemens

Thanks,

Artem