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

[Anthony Petrov]

Hi Peter,

Well, if an event is posted after the flush() call but before the lock() 
in detach(), two things may happen:

1. The event makes it to the event queue before detach() locks the 
pushPopLock. In which case the peekEvent() sees the event and prevents 
EDT detaching.

2. detach() takes the pushPopLock first, and hence the event is stuck in 
the PostEventQueue for a while. The EDT will be detached. However, once 
the lock is unlocked, the event will finally be delivered from 
PostEventQueue to the EventQueue, and the EDT must be re-created in this 
case to process the new event.

So other than recreating the EDT on a new thread, it's not a big deal I 
think.

--
best regards,
Anthony

On 8/31/2012 1:21 AM, Peter Levart wrote:
> Hi Oleg, Artem an others
> 
>  
> 
> If you are concerned about loosing events in 
> EventQueue.detachDispatchThread (a small window between returning from 
> SunToolkit.flushPendingEvents() and acquire-ing the pushPopLock) and can 
> add to SunToolkit an overloaded static flushPendingEvents method that 
> takes an aditional Runnable parameter called "afterFlushCallback", then 
> the pushPopLock guarded section of detachDispatchThread can be passed as 
> such afterFlushCallback to SunToolkit.flushPendingEvents.
> 
>  
> 
> This way you reverse the order of locks and prevent dead-lock while 
> guarantee-ing that no events get discarded...
> 
>  
> 
> Regards, Peter
> 
>  
> 
> On Thursday, August 30, 2012 04:42:00 PM Artem Ananiev wrote:
> 
>  > Hi, Anthony,
> 
>  >
> 
>  > 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
> 
>  > > 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.
> 
>  >
> 
>  > yes, this is a known issue: we don't guarantee that no new events are
> 
>  > posted between flush() and lock(). If this happens, we'll re-create
> 
>  > event dispatch thread.
> 
>  >
> 
>  > > What exactly prevents us from adding some synchronization to ensure 
> that
> 
>  > > the detaching only happens when there's really no pending events?
> 
>  >
> 
>  > As Oleg wrote, this is exactly the deadlock we're trying to resolve.
> 
>  > EQ.detachDispatchThread() was the only place where the order of locks
> 
>  > was pushPopLock->flushLock, while in other cases we flush events without
> 
>  > pushPopLock.
> 
>  >
> 
>  > > 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?
> 
>  >
> 
>  > As David correctly wrote, isThreadLocalFlushing is a ThreadLocal object,
> 
>  > which is thread-safe. isFlushing is used to synchronize access from
> 
>  > multiple threads, isThreadLockFlushing is to prevent EQ.postEvent() to
> 
>  > be called recursively.
> 
>  >
> 
>  > The only valid comment is that isThreadLocalFlushing should be set to
> 
>  > false in the "finally" block. Oleg will include this change into the
> 
>  > next version of the fix.
> 
>  >
> 
>  > > Overall, I find the current synchronization scheme in flush() very,
> 
>  > > *very* (and I mean it) confusing. Can we simplify it somehow?
> 
>  >
> 
>  > The current Oleg's fix is the simplest yet (almost) backwards compatible
> 
>  > solution we've found. If you have another ideas, please, let us know :)
> 
>  >
> 
>  > Thanks,
> 
>  >
> 
>  > Artem
> 
>  >
> 
>  > > --
> 
>  > > best regards,
> 
>  > > Anthony
> 
>  > >
> 
>  > > 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/>
>