Hi, Anthony,
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
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.
yes, this is a known issue: we don't guarantee that no new events are
posted between flush() and lock(). If this happens, we'll re-create
event dispatch thread.
What exactly prevents us from adding some synchronization to ensure that
the detaching only happens when there's really no pending events?
As Oleg wrote, this is exactly the deadlock we're trying to resolve.
EQ.detachDispatchThread() was the only place where the order of locks
was pushPopLock->flushLock, while in other cases we flush events without
pushPopLock.
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?
As David correctly wrote, isThreadLocalFlushing is a ThreadLocal object,
which is thread-safe. isFlushing is used to synchronize access from
multiple threads, isThreadLockFlushing is to prevent EQ.postEvent() to
be called recursively.
The only valid comment is that isThreadLocalFlushing should be set to
false in the "finally" block. Oleg will include this change into the
next version of the fix.
Overall, I find the current synchronization scheme in flush() very,
*very* (and I mean it) confusing. Can we simplify it somehow?
The current Oleg's fix is the simplest yet (almost) backwards compatible
solution we've found. If you have another ideas, please, let us know :)
Thanks,
Artem
--
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/>
