On Tuesday, September 11, 2012 01:25:39 AM Oleg Pekhovskiy wrote: > Hi Peter, > > your idea might work if drainTo() is used before while-cycle, > otherwise the order of events could be broken sometimes.
I don't know how. The flush() is synchronized! No two threads can execute while-poll at the same time. And LinkedBlockingQueue is used as FIFO, so it does not matter if toolkit thread posts any events concurrently - the order of events is the same. > > Also, usage of LinkedBlockingQueue could lead to performance decrease Internally it uses ReentrantLock, which in flush while-poll loop is acquired once per poll. Uncontended acquire is just a CAS. I don't think that in this context (GUI events) it presents any difference. So any approach is good-enough. > (especially with drainTo()). > > The class has more complicated logic entailing pitfalls in future. That might be true: http://stackoverflow.com/questions/12349881/why-linkedblockingqueuepoll-may-hang-up > > Thanks, > Oleg Regards, Peter > > 08.09.2012 21:39, Peter Levart wrote: > > 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/>
