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

[Anthony Petrov]

I see. After giving it more thought I don't see an easy solution for 
this issue, too. As long as we guarantee that the EDT is recreated 
should more events be posted, I don't see a big problem with this. So 
let's stick with the "Minimize..." approach then.

--
best regards,
Anthony

On 08/30/12 00:18, Oleg Pekhovskiy wrote:
> Hi Anthony,
>
> I see your concerns.
>
> As PostEventQueue.flush() method left unsynchronized,
> we potentially could return PostEventQueue.noEvents()
> and return check in EventQueue.detachDispatchThread()
> back to the condition.
> But is could increase the possibility of deadlock in future
> (with PostEventQueue & pushPopLock).
>
> Artem, what do you think?
>
> Thanks,
> Oleg
>
> 29.08.2012 15:22, Anthony Petrov wrote:
>> 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/>
>>>>
>>>>
>