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

[Anthony Petrov]

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

--
best regards,
Anthony

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