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

Peter Levart peter.levart at gmail.com
Sat Sep 8 10:39:11 PDT 2012


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/>



More information about the awt-dev mailing list