Hi David,
thank you for paying attention to that issue.
I need it checking the fix from different points of view.
I'll try to comment your thoughts:
1. We should differ PostEventQueue.postEvent and EventQueue.postEvent.
But in both cases the order of posted events are kept as they goes.
SunToolkit.flushLock guarantees that.
From the other way PostEventQueue is not public class and its flush()
method is called only from two classes: SunToolkit & SunToolkitSubclass.
Their method flushPendingEvents both use SunToolkit.flushLock.
2. In spite of the fact EDT continues calling pumpEvents no events would
be pumped because of EventDispatchThread.shutdown flag
which is checked in EventDispatchThread.pumpEventsForFilter method in
while-cycle condition that calls pumpOneEventForFilters.
Thanks,
Oleg
7/12/2012 5:41 AM, David Holmes wrote:
Hi Oleg,
"deadlock" always grabs my attention :)
On 12/07/2012 2:08 AM, Oleg Pekhovskiy wrote:
Thank you for the review, Anthony,
please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/7u6/7177040.2
I followed your suggestions there.
I also added 'volatile' for isFlushing because it could be accessed from
different threads.
I was about to mention that :)
I don't have the complete picture here but the actions of flush() no
longer "ensure the flush is completed before a new event can be posted
to this queue". The assurance may still be there in the way things are
called, but it not ensured by the flush() method.
It also seems that with this fix the interrupted EDT will not detach
due to the flush being in progress. The EDT will resume the pumpEvents
loop in its run() method. If the interrupt state of the EDT has not
been maintained then the fact it was interrupted (and should detach?)
is now lost. If it is maintained then presumably we will try to
detachDispatchThread again, in which case the EDT will enter a busy
polling loop trying, but failing, to detach, until the flush() has
actually completed.
Cheers,
David
Thanks,
Oleg
7/11/2012 7:35 PM, Anthony Petrov wrote:
Hi Oleg,
1. I suggest to rename isPending to isFlushing, as it reflects the
current state of the PostEventQueue more precisely.
2. I suggest to use the try{}finally{} pattern to set/reset the new
flag in the flush() method to ensure the flag is reset even if the
while() loop throws an exception.
Otherwise the fix looks good.
--
best regards,
Anthony
On 7/11/2012 7:24 PM, Oleg Pekhovskiy wrote:
Hi!
Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7177040
Webrev:
http://cr.openjdk.java.net/~bagiras/7u6/7177040.1
The idea of the fix is that there are two concurrent threads that try
to get two synchronization objects EventQueue.pushPopLock and
PostEventQueue itself.
For NetBeans these threads are (EDT & WarmUp) or (EDT & TimerQueue).
Problem happens when EDT is interrupted and goes to
EventQueue.detachDispatchThread() where EventQueue.pushPopLock is
owned and
EDT is waiting to own PostEventQueue when calling
SunToolkit.isPostEventQueueEmpty() -> PostEventQueue.noEvents().
At the same time another thread calls EventQueue.postEvent() that
calls EventQueue.flushPendingEvents() -> PostEventQueue.flush() where
PostEventQueue is owned
and another thread is waiting to own EventQueue.pushPopLock when
calling EventQueue.postEvent() -> EventQueue.postEventPrivate().
To avoid potential deadlock I moved synchronization out of
postEvent()'s cycle in PostEventQueue.flush(),
but to be clear about the existence of Events that are not posted to
EventQueue yet I added PostEventQueue.isPending flag that is true
until the end of the cycle.
There are only two classes that utilize PostEventQueue.flush()
method: SunToolkit.flushPendingEvents() and
SunToolkitSubclass.flushPendingEvents().
They are both synchronized by static SunToolkit.flushLock. That
eliminates the situation when we have overlapped calls of
PostEventQueue.flush().
Thanks,
Oleg
