<AWT Dev> [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue
Artem Ananiev
artem.ananiev at oracle.com
Mon Sep 10 04:27:34 PDT 2012
Hi, David,
see my comments inline.
On 9/8/2012 4:44 AM, David Holmes wrote:
> 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.
Agree.
> ---
>
> 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.
The "flushThread" field is not volatile, so we can't check its value
outside of synchronized blocks.
> 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.
Agree, it looks redundant.
Thanks,
Artem
> 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