Anthony, David,
thank you for the review!
Your comments are reasonable,
I'll try to apply them both.
Thanks,
Oleg
29.08.2012 16:12, David Holmes wrote:
> 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.
Right!
Also there is a problem with the InterruptedException handling. Let
thread A set isFlushing and be busy flushing. Then let Thread B call
wait() but be interrupted. Thread B will enter the finally block grab
the lock and set isFlushing to false, even though Thread A is actively
flushing! We don't want the finally block to execute if
InterruptedException is caught.
David
On 29/08/2012 10:02 PM, Anthony Petrov wrote:
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/>
