<AWT Dev> [7u8] Review request for 7177040: Deadlock between PostEventQueue.noEvents, EventQueue.isDispatchThread and SwingUtilities.invokeLater

[Peter Levart]

That's why I thought that fixing 7177040 and 7186109 (the last one by removing 
flushLock in SunToolkit and moving synchronization/recursion prevention into 
PostEventQueue) in one go would be worth considering...

Regards, Peter


On Friday, July 27, 2012 12:21:03 AM David Holmes wrote:
> On 26/07/2012 11:59 PM, Oleg Pekhovskiy wrote:
> > it's thread-safe indirectly. SunToolkit.flushPendingEvents() &
> > SunToolkitSubclass.flushPendingEvents() are the only methods that call
> > PostEventQueue.flush().
> > They both have flushLock synchronization object that guarantees
> > synchronized call of PostEventQueue.flush().
> 
> Yes but it is that undocumented indirection that concerns me. If someone
> decides to remove those outer locks they may not realize there is a
> lurking race here.
> 
> David
> 
> > Thanks,
> > Oleg
> > 
> > 7/26/2012 4:08 PM, David Holmes wrote:
> >> This fix still makes me uncomfortable because flush() is not
> >> thread-safe any more. If two threads can flush() then the first can
> >> clear the isFlushing state (in the finally block) that has been set by
> >> the second (in the synchronized block).
> >> 
> >> David
> >> 
> >> On 24/07/2012 9:42 PM, Peter Levart wrote:
> >>> Hi Oleg,
> >>> 
> >>> This is just cosmetics, but:
> >>> 
> >>> SunToolkit:
> >>> public synchronized boolean noEvents() {
> >>> return queueHead == null && !isFlushing;
> >>> }
> >>> 
> >>> ... a thread calling noEvents could see occasional "spikes" of false
> >>> return even though there is no flushing being performed (when other
> >>> thread is calling flush on an empty PostEventQueue).
> >>> 
> >>> Improved flush method would look like this:
> >>> 
> >>> public void flush() {
> >>> EventQueueItem tempQueue;
> >>> synchronized (this) {
> >>> tempQueue = queueHead;
> >>> queueHead = queueTail = null;
> >>> isFlushing =*/(tempQueue != null)/*;
> >>> 
> >>> }
> >>> try {
> >>> while (tempQueue != null) {
> >>> eventQueue.postEvent(tempQueue.event);
> >>> tempQueue = tempQueue.next;
> >>> }
> >>> }
> >>> finally {
> >>> isFlushing = false;
> >>> }
> >>> }
> >>> 
> >>> 
> >>> Regards, Peter
> >>> 
> >>> 2012/7/23 Oleg Pekhovskiy <oleg.pekhovskiy at oracle.com
> >>> <mailto:oleg.pekhovskiy at oracle.com>>
> >>> 
> >>> Hi!
> >>> 
> >>> Please review this back-port being already pushed to jdk8 but
> >>> deferred for 7u6.
> >>> 
> >>> Bug:
> >>> http://bugs.sun.com/view_bug.__do?bug_id=7177040
> >>> <http://bugs.sun.com/view_bug.do?bug_id=7177040>
> >>> 
> >>> Webrev:
> >>> http://cr.openjdk.java.net/~__bagiras/7u8/7177040.1
> >>> <http://cr.openjdk.java.net/~bagiras/7u8/7177040.1>
> >>> 
> >>> Review thread for 7u6:
> >>> http://mail.openjdk.java.net/__pipermail/awt-dev/2012-July/__003106.html
> >>> <http://mail.openjdk.java.net/pipermail/awt-dev/2012-July/003106.html>
> >>> 
> >>> Reviewers 7u6 & 8:
> >>> Anthony Petrov, Anton Tarasov
> >>> 
> >>> Thanks,
> >>> Oleg