Hi Oleg,
The reasoning in my previous message, that I made just before going to bed,
was not entirely correct. I made a mistake which was obvious in the morning.
But even if .notifyAll() is correctly interpreted, the results are similar.
So, once again:
Imagine two threads (t1, t2) calling concurrently EventQueue.postEvent() -
each with it's own event (e1, e2):
It can happen that t1:
- acquires the monitor in PostEventQueue 1st,
- establishes the flushThread variable,
- takes 1st snapshot of events,
- releases the monitor,
- flushes 1st snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via "notifyAll()",
- releases the monitor, ...
when t2 that was waiting, wakes up:
- acquires the monitor
- establishes the flushThread variable,
- takes a 2nd snapshot of events,
- releases the monitor,
- flushes 2nd snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via "notifyAll()",
- releases the monitor,
- posts e2 to the EventQueue, ...
and only after that t1 gets a chance to:
- post e1 to the EventQueue
The order of events that end up in EventQueue would still be as follows:
1st snapshot (taken by t1) of toolkit events,
2nd snapshot (taken by t2) of toolkit events,
e2 (posted by t2),
e1 (posted by t1)
where "2nd snapshot" are toolkit events that were "born" after e1.
Probability of such a scenario is very low, but a higher probability is that
at least some events from 2nd snapshot get inserted into EventQueue before e1.
The point I was trying to make is that as it is done currently, events that
end up in the EventQueue are only roughly ordered according to the time at
which they are posted to the queues by different threads. Ordering among
threads is not specified (the events don't have a timestamp or a sequence
allocated) Ordering is only maintained among events posted from the same
thread.
Different approaches are only more or less fair as to the ordering by "real"
time.
Regards, Peter
On Wednesday, September 12, 2012 12:02:17 AM Peter Levart wrote:
Hi Oleg,
On Tuesday, September 11, 2012 04:03:01 PM Oleg Pekhovskiy wrote:
Hi Peter,
please, see my comments below...
Thanks,
Oleg
9/11/2012 9:42 AM, Peter Levart wrote:
On Tuesday, September 11, 2012 01:25:39 AM Oleg Pekhovskiy wrote:
Hi Peter,
your idea might work if drainTo() is used before while-cycle,
otherwise the order of events could be broken sometimes.
I don't know how. The flush() is synchronized! No two threads can
execute
while-poll at the same time. And LinkedBlockingQueue is used as FIFO, so
it does not matter if toolkit thread posts any events concurrently - the
order of events is the same.
Imagine the situation when flushing caused by EventQueue.postEvent()
(called from any thread) is in progress.
Toolkit thread calls PostEventQueue.postEvent() while poll() is being
calling over and over again.
As a result the message posted by Toolkit thread would be posted to
EventQueue within current flushing cycle.
That would happen before posting of the event that caused flushing (see
EventQueue.postEvent()) and would violate the order of messages.
There's no absolute guarantee anyway since there's allways a window between
SunToolkit.flushPendingEvents() and acquire-ing a pushPopLock where anything
can happen...
Imagine two threads (t1, t2) calling concurrently EventQueue.postEvent() -
each with it's own event (e1, e2):
It can happen that t1:
- acquires the monitor in PostEventQueue 1st,
- establishes the flushThread variable,
- takes 1st snapshot of events,
- releases the monitor,
- flushes 1st snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via "notifyAll()" which temporarily releases the monitor, ...
when t2 that was waiting, wakes up:
- acquires the monitor
- establishes the flushThread variable,
- takes a 2nd snapshot of events,
- releases the monitor,
- flushes 2nd snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via "notifyAll()" which temporarily releases the monitor,
- reacquires the monitor,
- releases the monitor
- posts e2 to the EventQueue, ...
and only after that t1 gets a chance to:
- re-acquire the monitor
- release the monitor
- post e1 to the EventQueue
The order of events that end up in EventQueue is then as follows:
1st snapshot (taken by t1) of toolkit events,
2nd snapshot (taken by t2) of toolkit events,
e2 (posted by t2),
e1 (posted by t1)
where "2nd snapshot" are toolkit events that were "born" after e1.
Nobody guarantees that the event posted by EventQueue.postEvent will be the
1st event to arrive into the EventQueue after the snapshot of toolkit events
taken by the same thread at just the same time.
This order could only be maintained if SunToolkit.flushPendingEvents() was
allways called as part of the critical section in the EventQueue guarded by
pushPopLock. But then this would have to be guaranteed allways or there is a
chance of deadlock.
(It's true that flushing is not done while holding a monitor on
PostEventQueue any more, but flushingThread variable and a wait loop at the
begginig of flush() actualy constitue a self-made mutex lock which guards
the section containing flushing loop and can equally so participate in a
dead-lock situation.)
Since SunToolkit.flushPendingEvents() is public, anyone can call it, so this
is dangerous...
Regards, Peter
P.S. I'm just thinking loud:
What about a scheme where there was a special "copying" thread employed for
transfering events from PostEventQueue to EventQueue? No
SunToolkit.flushPendingEvents() method is needed any more then.
Also, usage of LinkedBlockingQueue could lead to performance decrease
Internally it uses ReentrantLock, which in flush while-poll loop is
acquired once per poll. Uncontended acquire is just a CAS. I don't think
that in this context (GUI events) it presents any difference. So any
approach is good-enough.>
(especially with drainTo()).
The class has more complicated logic entailing pitfalls in future.
That might be true:
http://stackoverflow.com/questions/12349881/why-linkedblockingqueuepoll-
ma
y-hang-up
I'm not denying that CAS in most cases is faster than locking. But if
you look at the implementation of LinkedBlockingQueue you'll see some
unnecessary things (for our case) as this class is quite common.
Thanks,
Oleg
Regards, Peter
08.09.2012 21:39, Peter Levart wrote:
Hi Oleg,
I still think that there is room for simplification. Now that the
flush
can be synchronized again (because EventQueue.detatchDispatchThread is
not calling SunToolkit.PostEventQueue's synchronized methods while
holding pushPopLock), we just have to make sure that toolkit thread is
not blocked by flushing.
Here's a simplified PostEventQueue that does that:
class PostEventQueue {
private final Queue<AWTEvent> queue = new
LinkedBlockingQueue<>();
//
unbounded private final EventQueue eventQueue;
private boolean isFlushing;
PostEventQueue(EventQueue eq) {
eventQueue = eq;
}
public synchronized void flush() {
if (isFlushing)
return;
isFlushing = true;
try {
AWTEvent event;
while ((event = queue.poll()) != null)
eventQueue.postEvent(event);
}
finally {
isFlushing = false;
}
}
/*
* Enqueue an AWTEvent to be posted to the Java EventQueue.
*/
void postEvent(AWTEvent event) {
queue.offer(event);
SunToolkit.wakeupEventQueue(eventQueue, event.getSource() ==
AWTAutoShutdown.getInstance());>
}
}
This implementation also finishes the flush in the presence of
interrupts...
Peter
On Saturday, September 08, 2012 04:09:40 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.
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/.
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?
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/>
