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

[Oleg Pekhovskiy]

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()

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.

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