Hi David,
On 8/29/2012 3:45 PM, David Holmes wrote:
I took a look at the last incarnation of this so let me see if I can
follow the new scheme.
On 29/08/2012 9: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.
If a new event is posted before the lock() then within the locked region
won't peekEvent() see it and so avoid the detach?
peekEvent() checks the event queue only, while the posted event may be
stuck in the PostEventQueue. The flushPendingEvents() actually posts the
events from the PostEventQueue to the EventQueue.
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?
isThreadLocalFlushing is a ThreadLocal so no synchronization is needed.
I presume it is used to prevent re-entrant/recursive calls to flush()
when calling postEvent.
The isFlushing variable is the global flag to coordinate flushing across
multiple threads. It has to be set and cleared in synchronized blocks,
but the synchronization lock has to be dropped when calling postEvent to
avoid deadlocks. So a thread acquires the lock and checks if flushing is
in progress, and if so it waits. Else/then it updates isFlushing to
indicate if that thread is doing flushing and releases the lock. It then
does any flushing needed, reacquires the lock, sets isFlushing to false
and notifies any other threads who may be waiting.
Overall, I find the current synchronization scheme in flush() very,
*very* (and I mean it) confusing. Can we simplify it somehow?
This seems like a reasonable protocol to coordinate multiple flushers
whilst dropping the synchronization lock when posting events. The actual
coordination might be simpler to understand if expressed using a
Semaphore - but I think the semantics would be the same.
Ah, I see. Thanks for the insight. It now looks much clearer. I think
that the final isThreadLocalFlushing.set(false); must be in the
finally{} block, though.
--
best regards,
Anthony
David
--
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/>
