<AWT Dev> Endless loop in EventDispatchThread - proposed solution

[David Holmes]

On 26/08/2011 2:39 AM, Clemens Eisserer wrote:
> Hi David,
>
> Thanks for your feedback.
>
>
>     You changed from Lock to ReentrantLock so that you could use
>     isHeldByCurrentThread(), but that locks in (pardon the pun) the kind of
>     Lock that AppContext must use.
>     You should add a comment explaining why the check for
>     isHeldByCurrentThread is needed - and that if things are done right at a
>     higher-level we should never need the stop() to break out of await().
>
>
> I will add a comment about that, I also took the lock-in to ReentrantLock as
> a trade-of as I couldn't think of another lock-implementation that would be
> more suitable for the job of the pushpopLock.
> I think things are done the right-way at a higher level (as with the "full"
> patch, the code should *never* catch ThreadDeath execpt some user-code in an
> InvocationEvent does strange things - which we can't control).
> Also the "normal" way of termination doesn't depend on catching ThreadDeath
> or InterruptedException, as shutdown is set synchronously to true by the
> Thread calling AppContext.dispose().
>
> What bothers me is that code known to be good style and is recommended
> everywhere, simply hides an InterruptedException and causes an
> IllegalMontorState-Exception to be thrown - actually the
> isHeldByCurrentThread is just there to avoid ugly IllegalMonitorException

Yes I understand why you added the check. I'm not sure what you mean about 
hiding the InterruptedException though. As I understood the scenario the 
interrupt() breaks the thread out of await() by throwing IE, which is then 
caught at a higher-level (not sure exactly where), but because of further 
pending events the IE does not cause the EDT to detach/terminate but instead 
it continues to process events eventually calling await() again. Then the 
stop() gets issued which causes the await() to abort by throwing 
ThreadDeath, without reclaiming the Lock, and so the unlock in the finally 
clause throws IllegalMonitorStateException.

> Stack-Traces printed out - as its done when using ThreadPools with Applets.
> Java's monitor design took care of that. As far as I can see, a tryUnlock()
> method would solve those problems.

But it would be the wrong solution. You should (almost) never be unlocking 
a Lock that you didn't lock first. The problem here is asynchronous 
exceptions - pure and simple. But I think it is time to revisit this issue 
in AbstractQueuedSynchronizer.

>     Overall I'm still concerned that there is an issue in the overall design
>     that permits events to be queued even after a "shutdown" has been
>     logically initiated. With this patch those events won't get processed
>     and not knowing what they are I can't say whether this will be a problem
>     or not. It is a concern that the current code in detachDispatchThread says:
>     as it seems to indicate that the exact conditions for detachment are
>     unclear. Based on reading 4648733 I'm assuming that we have to keep the
>     event queue receiving events so that the  shutdown event can be posted
>     (as part of AWT auto-shutdown), and that then allows other events in.
>     The question remains as to whether those events should be processed even
>     when shutdown has been initiated.
>
>
> I am no AWT expert, but from how I interpret the old code, as soon as
> interrupt() has been called, it was not intended to dispatch further events
> (I don't think the isInterrupted() call was really ment that way).
> However, I wonder why AppContext.stopEventDispatchThreads() is never used in
> AppContext.dispose(), as it seems to provide a cleaner way for shutdown?

I'm no AWT expert either, it just concerns me when there is an introduced 
change in behaviour without a full understanding of the implications. We 
really need some input from an AWT event processing guru.

Cheers,
David

> Thanks, Clemens