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

[Peter Levart]

Hi Oleg,

Now that SunToolkit is never called from EventQueue while holding pushPopLock 
(not even from detatchDispatchThread - I saw you removed 
SunToolkit.isPostEventQueueEmpty() check), there's no need for flushing loop in 
PostEventQueue not to be simply synchronized again and be done with 
InterruptedExceptions and handlers, Am I right?

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/>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20120830/fc4bc112/attachment.html