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

[Oleg Pekhovskiy]

Hi Peter,

making PostEventQueue.flush() method 'synchronized' will block Toolkit 
thread during PostEventQueue.postEvent()
call, that is bad. In our case synchronization monitor is released on 
wait(), thus no blocking occurs.

Thanks,
Oleg

31.08.2012 1:01, Peter Levart wrote:
>
> If I'm right, then instead of using thread-local flag for 
> recursion-prevention, you can just re-instate a boolean flag:
>
> private boolean isFlushing = false;
>
> public synchronized void flush() {
>
> if (isFlushing) {
>
> // every EventQueue.postEvent calls us back - prevent recursion
>
> return;
>
> }
>
> isFlushing = true;
>
> try {
>
> EventQueueItem tempQueue = queueHead;
>
> queueHead = queueTail = null;
>
> while (tempQueue != null) {
>
> eventQueue.postEvent(tempQueue.event);
>
> tempQueue = tempQueue.next;
>
> }
>
> }
>
> }
>
> finally {
>
> isFlushing = false;
>
> }
>
> }
>
> Regards, Peter
>
> On Thursday, August 30, 2012 10:39:03 PM Peter Levart wrote:
>
> 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/20120831/5cb22982/attachment.html