[8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue (original) (raw)

Oleg Pekhovskiy oleg.pekhovskiy at oracle.com
Wed Aug 29 14:34:43 PDT 2012

Anthony, David, thank you for the review!

Your comments are reasonable, I'll try to apply them both.

Thanks, Oleg

29.08.2012 16:12, David Holmes wrote:

> Ah, I see. Thanks for the insight. It now looks much clearer. I think > that the final isThreadLocalFlushing.set(false); must be in the > finally{} block, though.

Right! Also there is a problem with the InterruptedException handling. Let thread A set isFlushing and be busy flushing. Then let Thread B call wait() but be interrupted. Thread B will enter the finally block grab the lock and set isFlushing to false, even though Thread A is actively flushing! We don't want the finally block to execute if InterruptedException is caught. David On 29/08/2012 10:02 PM, Anthony Petrov wrote: Hi David,

On 8/29/2012 3:45 PM, David Holmes wrote: I took a look at the last incarnation of this so let me see if I can follow the new scheme.

On 29/08/2012 9: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. If a new event is posted before the lock() then within the locked region won't peekEvent() see it and so avoid the detach? peekEvent() checks the event queue only, while the posted event may be stuck in the PostEventQueue. The flushPendingEvents() actually posts the events from the PostEventQueue to the EventQueue. 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? isThreadLocalFlushing is a ThreadLocal so no synchronization is needed. I presume it is used to prevent re-entrant/recursive calls to flush() when calling postEvent. The isFlushing variable is the global flag to coordinate flushing across multiple threads. It has to be set and cleared in synchronized blocks, but the synchronization lock has to be dropped when calling postEvent to avoid deadlocks. So a thread acquires the lock and checks if flushing is in progress, and if so it waits. Else/then it updates isFlushing to indicate if that thread is doing flushing and releases the lock. It then does any flushing needed, reacquires the lock, sets isFlushing to false and notifies any other threads who may be waiting. Overall, I find the current synchronization scheme in flush() very, very (and I mean it) confusing. Can we simplify it somehow? This seems like a reasonable protocol to coordinate multiple flushers whilst dropping the synchronization lock when posting events. The actual coordination might be simpler to understand if expressed using a Semaphore - but I think the semantics would be the same. Ah, I see. Thanks for the insight. It now looks much clearer. I think that the final isThreadLocalFlushing.set(false); must be in the finally{} block, though. -- best regards, Anthony

David -- 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/viewbug.do?bugid=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/>

More information about the awt-dev mailing list