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