<AWT Dev> Endless loop in EventDispatchThread - proposed solution

[David Holmes]

Hi Clemens,

On 21/08/2011 9:25 PM, Clemens Eisserer wrote:
> 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?

It seems to me that when the app context is being shutdown in this manner 
that part of that should be to purge/clear the event queue. To me that would 
be a more appropriate fix for this non-terminating situation.

>
> 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.

Right. The ThreadDeath gets seen while trying to acquire the Lock to return 
from await() and so the lock is not reacquired and so the subsequent 
unlock() throws IMSE. There's an open bug on this (6627356), against 
Condition.await, but like anything to do with async exceptions there are no 
easy solutions. I'd prefer if AppContext could stop using a method that's 
been deprecated for a decade (and which I'd love to physically remove in 
Java 8).

BTW the interrupt() should really be causing the await() to complete, but 
there must be contention on the Lock causing the stop() to be issued before 
the lock is acquired. Maybe that contention could be removed to allow the 
interrupt() to do all the work?

> 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.

Seems to me that there should be a simple variable that the run() method can 
check which gets set when the thread is expected to terminate. That way it 
doesn't matter whether it gets hit by the interrupt() or the stop(), it will 
terminate based on the flag.

But any changes to this code need deep scrutiny - I know many eyes have 
looked at it over the years. And there's no doubt a fair amount of 
historical baggage as well.

David

> Thanks, Clemens
>
>
>
>