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

[Oleg Pekhovskiy]

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