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

[David Holmes]

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