<AWT Dev> [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue
David Holmes
david.holmes at oracle.com
Fri Sep 7 17:44:03 PDT 2012
Hi Oleg,
I can't really comment on the higher-level logic/protocol here as I
can't track which thread does what, where and when.
In EventQueue.java this comment is no longer valid:
* Fix for 4648733. Check both the associated java event
* queue and the PostEventQueue.
*/
! if (!forceDetach && (peekEvent() != null)) {
as the PostEventQueue is not checked.
---
The flush() logic can be simplified somewhat as you can check for
recursion outside the sync block** and outside any try/finally:
public void flush() {
Thread newThread = Thread.currentThread();
// Avoid method recursion
if (newThread == flushThread) {
return;
}
try {
EventQueueItem tempQueue;
synchronized (this) {
// Wait for other threads' flushing
while (flushThread != null) {
wait();
}
// Skip everything if queue is empty
if (queueHead == null) {
notifyAll();
return;
}
// Remember flushing thread
flushThread = newThread;
tempQueue = queueHead;
queueHead = queueTail = null;
}
while (tempQueue != null) {
eventQueue.postEvent(tempQueue.event);
tempQueue = tempQueue.next;
}
}
catch (InterruptedException e) {
// Couldn't allow exception go up, so at least recover the flag
newThread.interrupt();
}
finally {
synchronized (this) {
// Forget flushing thread, inform other pending threads
if (newThread == flushThread) {
flushThread = null;
notifyAll();
}
}
}
}
** This is safe because a thread only ever writes its own value to
flushThread so even if it reads a stale value that value will either be
null or some other thread - either way it is not the current thread so
it proceeds with the main logic.
I also think you can move the try/finally to around the inner code that
does the event processing, and leave just the try/catch at the outer-level
public void flush() {
Thread newThread = Thread.currentThread();
// Avoid method recursion
if (newThread == flushThread) {
return;
}
try {
EventQueueItem tempQueue;
synchronized (this) {
// Wait for other threads' flushing
while (flushThread != null) {
wait();
}
// Skip everything if queue is empty
if (queueHead == null) {
notifyAll();
return;
}
// Remember flushing thread
flushThread = newThread;
tempQueue = queueHead;
queueHead = queueTail = null;
}
try {
while (tempQueue != null) {
eventQueue.postEvent(tempQueue.event);
tempQueue = tempQueue.next;
}
} finally {
// only the flushing thread can get here
synchronized (this) {
// Forget flushing thread, inform other pending threads
flushThread = null;
notifyAll();
}
}
}
catch (InterruptedException e) {
// Couldn't allow exception go up, so at least recover the flag
newThread.interrupt();
}
}
I'm also not convinced the notifyAll() is needed when you "Skip
everything". It's harmless from a functional perspective, but
unnecessary I think.
David
On 8/09/2012 10:09 AM, Oleg Pekhovskiy wrote:
> Artem, Anthony, David,
>
> please review the next version of fix:
> http://cr.openjdk.java.net/~bagiras/8/7186109.4/
>
> What's new in comparison with the previous version:
>
> 1. Removed "isThreadLocalFlushing" and "isFlashing", created
> "flushThread" instead
> (stores the thread that currently performs flushing).
> 2. Implemented both recursion and multi-thread block on "flushThread" only.
> 3. Added Thread.interrupt() to the catch block as outside we would like
> to know what happened in PostEventQueue.flush().
> We are not able to rethrow the exception because we couldn't change the
> signature (by adding "throwns")
> for all related methods (e.g. EventQueue.postEvent()).
>
> The fix successfully passed
> "closed/java/awt/EventQueue/PostEventOrderingTest.java" test.
>
> IMHO, the code became clearer.
>
> Looking forward to your comments!
>
> Thanks,
> Oleg
>
> 8/30/2012 9:19 PM, Oleg Pekhovskiy wrote:
>> There are also other 2 methods that will require 'throws
>> InterruptedException' or try-catch:
>> 1. EventQueue.postEvent()
>> 2. EventQueue.removeSourceEvents()
>>
>> Thanks,
>> Oleg
>>
>> 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/>
>>>>>>>>
>>>>>>>>
>>>>>
>>>
>>
>
More information about the awt-dev
mailing list