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

[Anthony Petrov]

I think you should re-throw the InterruptedException caught at flush(). 
BTW, the flush() method can be invoked on a thread other than EDT (at 
least in theory).

--
best regards,
Anthony

On 8/30/2012 9:01 PM, Oleg Pekhovskiy wrote:
> Hi,
> 
> I got another idea preparing the next version of fix.
> 
> Previously we didn't catch InterruptedException and stack unwinding took 
> place right up to
> try-catch section in EventDispatchThread.pumpOneEventForFilters().
> 
> So seems like it would be correct not eating that exception in 
> PostEventQueue.flush()
> but just check the state using isInterrupted() method and add 'throws 
> InterruptedException'
> to PostEventQueue.flush() and SunToolkit.flushPendingEvents() methods.
> 
> Thoughts?
> 
> Thanks,
> Oleg
> 
> 8/30/2012 5:20 PM, Anthony Petrov wrote:
>> 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/>
>>>>>>
>>>>>>
>>>
>