<AWT Dev> [8] Review request for 7186109: Simplify lock machinery for PostEventQueue & EventQueue

[David Holmes]

Hi Oleg,

I have no further comments.

Thanks,
David

On 13/09/2012 4:56 AM, Oleg Pekhovskiy wrote:
> 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/>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>