(original) (raw)
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/>