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

[Anthony Petrov]

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.

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?

--
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/>
> 
>