<AWT Dev> [7u6] Review request for 7177040: Deadlock between PostEventQueue.noEvents, EventQueue.isDispatchThread and SwingUtilities.invokeLater

[David Holmes]

On 13/07/2012 8:13 AM, Oleg Pekhovskiy wrote:
> Hi David,
>
> the condition you mentioned:
>
> if (!forceDetach && (peekEvent() != null) ||
> !SunToolkit.isPostEventQueueEmpty()) {
> return false;
> }
>
> could lead to understanding problem (braces are the best way to clarify).
>
> && has higher priority than ||, thus we could rewrite that condition
> like this:
>
> ( !forceDetach && (peekEvent() != null) ) ||
> !SunToolkit.isPostEventQueueEmpty()

Sorry - need to clean my glasses. I was seeing !f && ( X || Y)

:(

> That means SunToolkit.isPostEventQueueEmpty() is called when:
> forceDetach == true
> OR
> peekEvent() == null
> OR
> both above.
>
> Anthony has pushed the fix that changes (and simplifies) that place:
> http://cr.openjdk.java.net/~anthony/7u6-16-missingAWTThread-7162144.0/
> So maybe we should discuss the new code - the new behavior.

Yes I saw that patch too. All clear now.

Thanks,
David


> Thanks,
> Oleg
>
> 7/13/2012 1:10 AM, David Holmes wrote:
>> On 12/07/2012 10:18 PM, Anthony Petrov wrote:
>>> On 07/12/12 05:41, David Holmes wrote:
>>
>> <snip>
>>
>>>> It also seems that with this fix the interrupted EDT will not detach
>>>> due
>>>> to the flush being in progress. The EDT will resume the pumpEvents loop
>>>> in its run() method. If the interrupt state of the EDT has not been
>>>> maintained then the fact it was interrupted (and should detach?) is now
>>>> lost. If it is maintained then presumably we will try to
>>>> detachDispatchThread again, in which case the EDT will enter a busy
>>>> polling loop trying, but failing, to detach, until the flush() has
>>>> actually completed.
>>>
>>> When interrupt() is called on EDT, the shutdown flag is set, which is
>>> subsequently passed as the forceDetach parameter to
>>> detachDispatchingThread(). If the parameter is true, the detaching
>>> happens unconditionally. So this shouldn't be an issue.
>>
>> I don't quite follow this part. The logic is:
>>
>> if (!forceDetach && (peekEvent() != null) ||
>> !SunToolkit.isPostEventQueueEmpty()) {
>> return false;
>> }
>>
>> If forceDetach is true then we won't execute peekEvent() and
>> isPostEventQueueEmpty(), which means we would not have hit the
>> original deadlock.
>>
>> Oleg's response seems to indicate that we will indeed keep looping but
>> because of the values of various flags we won't do anything "useful".
>> But that is the kind of "busy wait" that I was referring to.
>>
>> Cheers,
>> David
>>
>>
>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>>
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>> Oleg
>>>>>
>>>>> 7/11/2012 7:35 PM, Anthony Petrov wrote:
>>>>>> Hi Oleg,
>>>>>>
>>>>>> 1. I suggest to rename isPending to isFlushing, as it reflects the
>>>>>> current state of the PostEventQueue more precisely.
>>>>>>
>>>>>> 2. I suggest to use the try{}finally{} pattern to set/reset the new
>>>>>> flag in the flush() method to ensure the flag is reset even if the
>>>>>> while() loop throws an exception.
>>>>>>
>>>>>> Otherwise the fix looks good.
>>>>>>
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>>
>>>>>> On 7/11/2012 7:24 PM, Oleg Pekhovskiy wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Please review the fix for CR:
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7177040
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~bagiras/7u6/7177040.1
>>>>>>>
>>>>>>> The idea of the fix is that there are two concurrent threads that
>>>>>>> try
>>>>>>> to get two synchronization objects EventQueue.pushPopLock and
>>>>>>> PostEventQueue itself.
>>>>>>> For NetBeans these threads are (EDT & WarmUp) or (EDT & TimerQueue).
>>>>>>> Problem happens when EDT is interrupted and goes to
>>>>>>> EventQueue.detachDispatchThread() where EventQueue.pushPopLock is
>>>>>>> owned and
>>>>>>> EDT is waiting to own PostEventQueue when calling
>>>>>>> SunToolkit.isPostEventQueueEmpty() -> PostEventQueue.noEvents().
>>>>>>> At the same time another thread calls EventQueue.postEvent() that
>>>>>>> calls EventQueue.flushPendingEvents() -> PostEventQueue.flush()
>>>>>>> where
>>>>>>> PostEventQueue is owned
>>>>>>> and another thread is waiting to own EventQueue.pushPopLock when
>>>>>>> calling EventQueue.postEvent() -> EventQueue.postEventPrivate().
>>>>>>>
>>>>>>> To avoid potential deadlock I moved synchronization out of
>>>>>>> postEvent()'s cycle in PostEventQueue.flush(),
>>>>>>> but to be clear about the existence of Events that are not posted to
>>>>>>> EventQueue yet I added PostEventQueue.isPending flag that is true
>>>>>>> until the end of the cycle.
>>>>>>>
>>>>>>> There are only two classes that utilize PostEventQueue.flush()
>>>>>>> method: SunToolkit.flushPendingEvents() and
>>>>>>> SunToolkitSubclass.flushPendingEvents().
>>>>>>> They are both synchronized by static SunToolkit.flushLock. That
>>>>>>> eliminates the situation when we have overlapped calls of
>>>>>>> PostEventQueue.flush().
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Oleg
>>>>>
>