<AWT Dev> [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue

[Peter Levart]

On Tuesday, September 11, 2012 01:25:39 AM Oleg Pekhovskiy wrote:
> Hi Peter,
> 
> your idea might work if drainTo() is used before while-cycle,
> otherwise the order of events could be broken sometimes.

I don't know how. The flush() is synchronized! No two threads can execute while-poll at the same time. And LinkedBlockingQueue is used as FIFO, so it does not matter if toolkit thread posts any events concurrently - the order of events is the same.

> 
> Also, usage of LinkedBlockingQueue could lead to performance decrease

Internally it uses ReentrantLock, which in flush while-poll loop is acquired once per poll. Uncontended acquire is just a CAS. I don't think that in this context (GUI events) it presents any difference. So any approach is good-enough.

> (especially with drainTo()).
> 
> The class has more complicated logic entailing pitfalls in future.

That might be true:

http://stackoverflow.com/questions/12349881/why-linkedblockingqueuepoll-may-hang-up

> 
> Thanks,
> Oleg

Regards, Peter

> 
> 08.09.2012 21:39, Peter Levart wrote:
> > Hi Oleg,
> > 
> > I still think that there is room for simplification. Now that the flush
> > can be synchronized again (because EventQueue.detatchDispatchThread is
> > not calling SunToolkit.PostEventQueue's synchronized methods while
> > holding pushPopLock), we just have to make sure that toolkit thread is
> > not blocked by flushing.
> > 
> > Here's a simplified PostEventQueue that does that:
> > 
> > class PostEventQueue {
> > 
> >     private final Queue<AWTEvent> queue = new LinkedBlockingQueue<>(); //
> >     unbounded private final EventQueue eventQueue;
> >     
> >     private boolean isFlushing;
> >     
> >     PostEventQueue(EventQueue eq) {
> >     
> >        eventQueue = eq;
> >     
> >     }
> >     
> >     public synchronized void flush() {
> >     
> >        if (isFlushing)
> >        
> >           return;
> >        
> >        isFlushing = true;
> >        try {
> >        
> >           AWTEvent event;
> >           while ((event = queue.poll()) != null)
> >           
> >              eventQueue.postEvent(event);
> >        
> >        }
> >        finally {
> >        
> >           isFlushing = false;
> >        
> >        }
> >     
> >     }
> >     
> >     /*
> >     * Enqueue an AWTEvent to be posted to the Java EventQueue.
> >     */
> >     void postEvent(AWTEvent event) {
> >     
> >        queue.offer(event);
> >        SunToolkit.wakeupEventQueue(eventQueue, event.getSource() ==
> >        AWTAutoShutdown.getInstance());>     
> >     }
> > 
> > }
> > 
> > 
> > 
> > This implementation also finishes the flush in the presence of
> > interrupts...
> > 
> > Peter
> > 
> > On Saturday, September 08, 2012 04:09:40 AM Oleg Pekhovskiy wrote:
> >> Artem, Anthony, David,
> >> 
> >> please review the next version of fix:
> >> http://cr.openjdk.java.net/~bagiras/8/7186109.4/
> >> 
> >> What's new in comparison with the previous version:
> >> 
> >> 1. Removed "isThreadLocalFlushing" and "isFlashing", created
> >> "flushThread" instead
> >> (stores the thread that currently performs flushing).
> >> 2. Implemented both recursion and multi-thread block on "flushThread"
> >> only.
> >> 3. Added Thread.interrupt() to the catch block as outside we would like
> >> to know what happened in PostEventQueue.flush().
> >> We are not able to rethrow the exception because we couldn't change the
> >> signature (by adding "throwns")
> >> for all related methods (e.g. EventQueue.postEvent()).
> >> 
> >> The fix successfully passed
> >> "closed/java/awt/EventQueue/PostEventOrderingTest.java" test.
> >> 
> >> IMHO, the code became clearer.
> >> 
> >> Looking forward to your comments!
> >> 
> >> Thanks,
> >> Oleg
> >> 
> >> 8/30/2012 9:19 PM, Oleg Pekhovskiy wrote:
> >>> There are also other 2 methods that will require 'throws
> >>> InterruptedException' or try-catch:
> >>> 1. EventQueue.postEvent()
> >>> 2. EventQueue.removeSourceEvents()
> >>> 
> >>> Thanks,
> >>> Oleg
> >>> 
> >>> 8/30/2012 9:01 PM, Oleg Pekhovskiy wrote:
> >>>> Hi,
> >>>> 
> >>>> I got another idea preparing the next version of fix.
> >>>> 
> >>>> Previously we didn't catch InterruptedException and stack unwinding
> >>>> took place right up to
> >>>> try-catch section in EventDispatchThread.pumpOneEventForFilters().
> >>>> 
> >>>> So seems like it would be correct not eating that exception in
> >>>> PostEventQueue.flush()
> >>>> but just check the state using isInterrupted() method and add 'throws
> >>>> InterruptedException'
> >>>> to PostEventQueue.flush() and SunToolkit.flushPendingEvents() methods.
> >>>> 
> >>>> Thoughts?
> >>>> 
> >>>> Thanks,
> >>>> Oleg
> >>>> 
> >>>> 8/30/2012 5:20 PM, Anthony Petrov wrote:
> >>>>> I see. After giving it more thought I don't see an easy solution for
> >>>>> this issue, too. As long as we guarantee that the EDT is recreated
> >>>>> should more events be posted, I don't see a big problem with this.
> >>>>> So let's stick with the "Minimize..." approach then.
> >>>>> 
> >>>>> On 08/30/12 00:18, Oleg Pekhovskiy wrote:
> >>>>>> Hi Anthony,
> >>>>>> 
> >>>>>> I see your concerns.
> >>>>>> 
> >>>>>> As PostEventQueue.flush() method left unsynchronized,
> >>>>>> we potentially could return PostEventQueue.noEvents()
> >>>>>> and return check in EventQueue.detachDispatchThread()
> >>>>>> back to the condition.
> >>>>>> But is could increase the possibility of deadlock in future
> >>>>>> (with PostEventQueue & pushPopLock).
> >>>>>> 
> >>>>>> Artem, what do you think?
> >>>>>> 
> >>>>>> Thanks,
> >>>>>> Oleg
> >>>>>> 
> >>>>>> 29.08.2012 15:22, Anthony Petrov wrote:
> >>>>>>> On 8/29/2012 3:08 PM, Anthony Petrov wrote:
> >>>>>>>> Hi Oleg,
> >>>>>>>> 
> >>>>>>>> I'm still concerned about the following:
> >>>>>>>> 
> >>>>>>>> detachDispatchThread()
> >>>>>>>> {
> >>>>>>>> flush();
> >>>>>>>> lock();
> >>>>>>>> // possibly detach
> >>>>>>>> unlock();
> >>>>>>>> }
> >>>>>>>> 
> >>>>>>>> at EventQueue.java. What if an even get posted to the queue after
> >>>>>>>> the
> >>>>>>> 
> >>>>>>> A typo: s/even get/event gets/.
> >>>>>>> 
> >>>>>>>> flush() returns but before we even acquired the lock? We may still
> >>>>>>>> end up with a situation when we detach the thread while in fact
> >>>>>>>> there
> >>>>>>>> are some pending events present, which actually contradicts the
> >>>>>>>> current logic of the detach() method. I see that you say "Minimize
> >>>>>>>> discard possibility" in the comment instead of "Prevent ...", but I
> >>>>>>>> feel uncomfortable with this actually.
> >>>>>>>> 
> >>>>>>>> What exactly prevents us from adding some synchronization to ensure
> >>>>>>>> that the detaching only happens when there's really no pending
> >>>>>>>> events?
> >>>>>>>> 
> >>>>>>>> SunToolkit.java:
> >>>>>>>>> 2120 Boolean b = isThreadLocalFlushing.get();
> >>>>>>>>> 2121 if (b != null && b) {
> >>>>>>>>> 2122 return;
> >>>>>>>>> 2123 }
> >>>>>>>>> 2124 2125 isThreadLocalFlushing.set(true);
> >>>>>>>>> 2126 try {
> >>>>>>>> 
> >>>>>>>> How does access to the isThreadLocalFlushing synchronized? What
> >>>>>>>> happens if the flush() is being invoked from two different threads
> >>>>>>>> for the same post event queue? Why do we have two "isFlushing"
> >>>>>>>> flags?
> >>>>>>>> Can we collapse them into one? Why is the isFlushing set/reset in
> >>>>>>>> two
> >>>>>>>> disjunct synchronized(){} blocks?
> >>>>>>>> 
> >>>>>>>> Overall, I find the current synchronization scheme in flush() very,
> >>>>>>>> *very* (and I mean it) confusing. Can we simplify it somehow?
> >>>>>>>> 
> >>>>>>>> On 8/28/2012 6:33 PM, Oleg Pekhovskiy wrote:
> >>>>>>>>> Hi Artem, Anthony,
> >>>>>>>>> 
> >>>>>>>>> thank you for your proposals!
> >>>>>>>>> 
> >>>>>>>>> We with Artem also had off-line discussion,
> >>>>>>>>> so as a result I prepared improved version of fix:
> >>>>>>>>> http://cr.openjdk.java.net/~bagiras/8/7186109.3/
> >>>>>>>>> 
> >>>>>>>>> What was done:
> >>>>>>>>> 1. EventQueue.detachDispatchThread(): moved
> >>>>>>>>> SunToolkit.flushPnedingEvents() above the comments and added a
> >>>>>>>>> separate comment to it.
> >>>>>>>>> 2. Moved SunToolkitSubclass.flushPendingEvents(AppContext)
> >>>>>>>>> method to
> >>>>>>>>> SunToolkit. Deleted SunToolkitSubclass.
> >>>>>>>>> 3. Moved isFlushingPendingEvents to PostEventQueue with the new
> >>>>>>>>> name
> >>>>>>>>> - isThreadLocalFlushing and made it ThreadLocal.
> >>>>>>>>> 4. Left PostEventQueue.flush() unsynchronized and created
> >>>>>>>>> wait()-notifyAll() synchronization mechanism to avoid blocking of
> >>>>>>>>> PostEventQueue.postEvent().
> >>>>>>>>> 
> >>>>>>>>> Looking forward to your comments!
> >>>>>>>>> 
> >>>>>>>>> Thanks,
> >>>>>>>>> Oleg
> >>>>>>>>> 
> >>>>>>>>> 20.08.2012 20:20, Artem Ananiev wrote:
> >>>>>>>>>> Hi, Oleg,
> >>>>>>>>>> 
> >>>>>>>>>> here are a few comments:
> >>>>>>>>>> 
> >>>>>>>>>> 1. What is the reason of keeping "isFlushingPendingEvents" in
> >>>>>>>>>> SunToolkit, given that PEQ.flush() is synchronized (and therefore
> >>>>>>>>>> serialized) anyway?
> >>>>>>>>>> 
> >>>>>>>>>> 2. flushPendingEvents(AppContext) may be moved directly to
> >>>>>>>>>> SunToolkit, so we don't need a separate sun-class for that.
> >>>>>>>>>> 
> >>>>>>>>>> 3. EQ.java:1035-1040 - this comment is obsolete and must be
> >>>>>>>>>> replaced by another one.
> >>>>>>>>>> 
> >>>>>>>>>> Thanks,
> >>>>>>>>>> 
> >>>>>>>>>> Artem
> >>>>>>>>>> 
> >>>>>>>>>> On 8/17/2012 4:49 PM, Oleg Pekhovskiy wrote:
> >>>>>>>>>>> Hi!
> >>>>>>>>>>> 
> >>>>>>>>>>> Please review the fix for CR:
> >>>>>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7186109
> >>>>>>>>>>> 
> >>>>>>>>>>> Webrev:
> >>>>>>>>>>> http://cr.openjdk.java.net/~bagiras/8/7186109.1/
> >>>>>>>>>>> 
> >>>>>>>>>>> The following changes were made:
> >>>>>>>>>>> 1. Removed flushLock from SunToolkit.flushPendingEvent()
> >>>>>>>>>>> 2. Returned method PostEventQueue.flush() as 'synchronized' back
> >>>>>>>>>>> 3. Added call of SunToolkit.flushPendingEvents() to
> >>>>>>>>>>> EventQueue.detachDispatchThread(),
> >>>>>>>>>>> right before pushPopLock.lock()
> >>>>>>>>>>> 4. Removed !SunToolkit.isPostEventQueueEmpty() check from
> >>>>>>>>>>> EventQueue.detachDispatchThread()
> >>>>>>>>>>> 5. Removed SunToolkit.isPostEventQueueEmpty() &
> >>>>>>>>>>> PostEventQueue.noEvents();
> >>>>>>>>>>> 
> >>>>>>>>>>> Thanks,
> >>>>>>>>>>> Oleg
> >>>>>>>>>>> <http://cr.openjdk.java.net/%7Ebagiras/8/7186109.1/>