<AWT Dev> [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue
Oleg Pekhovskiy
oleg.pekhovskiy at oracle.com
Wed Sep 12 11:56:03 PDT 2012
Hi!
please look at the next version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.5/
What's new:
1. Moved "finally" block below "while-cycle".
2. Removed redundant notifyAll().
2. Added PostEventOrderingTest.java to the tests.
3. Removed "Fix for 4648733..." comment.
I left recursion check inside synchronized block to avoid making
flushThread "volatile"
or confusing people with unsynchronized usage of non-volatile variable,
in spite of the fact
it works properly.
Thanks,
Oleg
9/10/2012 3:27 PM, Artem Ananiev wrote:
> 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