On 22/08/2011 8:17 PM, Clemens Eisserer wrote:
Thanks again for taking a look at my comments and for your patience :)
Threads are my core interest :)
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.
Could be, I have to admit I am not an EventQueue expert :/
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?
There is at least one case when Thread.stop is required to halt the EDT:
If there are events added to the queue, after await() threw the
InterruptedException. In this case the EDT won't terminate and tries to
dispatch the remaining events, which it does until the queue is empty, and
then again it waits in await().
(the isInterrupted()-checks are a bit meaningless, because after an
InterruptedException is thrown in getNextEvent, isInterrupted() returns
false again)
There is no second Thread.interrupt(), so ThreadDeath is thrown when
Thread.stop() is called. (which currently leads to spinning, as
inInterrupted() returns true when Thread.stop has been called and
threadDeathCaught remains false forever)
Ah I see. So this would again be solved by purging the event queue. I'm no
expert on the event system per se so it may be that events can continue to
be generated after the decision to dispose of the app context has been made
- otherwise I would expect no further events and so clearing the queue would
allow the interrupt (plus variable setting) to act as the termination
indicator for the EDT.
David
I experienced this issue only on the highly loaded (and swapping, but not
OOM) server, that would explain the timing issues.
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.
Jep, I agree. Additionally, when pending events are dispatched during
shut-down, the queue should not block waiting for new events.
That async exceptions could even be consumed by bad-behaving user-code (e.g.
some Thread.sleep wrapped in a catch-block), a variable seems to be a much
cleaner solution.
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.
I'll prepare two patches:
- A simple one, which just restores pre-java7 behaviour, working arround
that IllegalMonitorStateException
- A more in-deptch change, trying not to rely on AsynExeptions for thread
termination.
Thanks again, Clemens
