Hi Anton,
the code was changed according to our offline discussion:
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.01/
added a test case for deterministic scenario
bug root cause description extended
--Semyon
On 3/27/2015 5:32 PM, Anton V. Tarasov wrote:
Hi Semyon,
It would be fine if you describe the case when the enter/exit methods
are not thread safe, that is the bug you're fixing. It's always
helpful to track all the essential details in the bug report.
AFAICS, the case is this:
a) "enter" is called on non-dispatch thread
b) the thread scheduler suspends it at, say, the point right after
"keepBlockingEDT" is set to true (line 177).
c) "exit" is called, "keepBlockingEDT" is set to false,
"wakingRunnable" is posted to EDT and gets executed on EDT,
"keepBlockingCT" is set to false
b) "enter" continues execution, "keepBlockingCT" is set to true,
getTreeLock().wait() potentially makes the thread sleep endlessly
Is this correct?
WRT the fix. It's actually a question when "exit" is considered to be
prematurely called. Consider the following:
a) "exit" is called first on EDT
b) at line 305 "keepBlockingEDT" is checked as false
c) say, at line 310 "enter" is called on EDT and is starting pumping
events
d) "exit" continues and wakes "enter" up
From my point of view, this situation looks functionally the same as
if "enter" and "exit" would be called in the correct order.
What about simplifying this logic?
public boolean exit() {
boolean res = false;
synchronized (getTreeLock()) {
res = keepBlockingEDT.getAndSet(false);
afterExit.set(true); // new atomic var
}
wakeupEDT();
return res;
}
Will this scheme work, provided that you use "afterExit" in "enter"
appropriately?
getTreeLock() should be called at a narrow possible scope. At least I
wouldn't include wakeupEDT() into it, unless it's justified. Even
then, I would consider adding another private lock for the sake of
synchronizing "enter" and "exit".
Also, what's the reason of the call at the following line?
325 keepBlockingEDT.set(false);
And the same question here:
326 if (keepBlockingCT.get()) {
327 // Notify only if enter() waiting in non-dispatch
thread
What's wrong if you call getTreeLock().notifyAll() without checking
keepBlockingCT?
WRT the test. Does it really call enter/exit in different order, on
your machine at least? Shouldn't be some test cases where the order is
guaranted?
Thanks,
Anton.
On 26.03.2015 17:49, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9.
Bug: https://bugs.openjdk.java.net/browse/JDK-6980209#comment-13623256
Webrev:
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.00/
***The root cause:
There are 2 issues in WaitDispatchSupport:
1. If exit() is called before the secondary loop just silently never
ends. That is pointed in the summary.
2. In the same spec it is proposed to call exit() and enter()
concurrently but they are not thread-safe. That is an additional issue.
To fix 1: I support Artem's proposal for enter() to return false
immidiately in case if disordered exit() detected.
To fix 2: WaitDispatchSupport need to be reworked to allow concurrent
execution. Unfortunately we cannot synchronize EDT with another
thread, so the secondary loop launching cannot be run atomically. And
this is very sensitive area because of potential possibility to block
EDT. Right testing of the fix is very desirable.
***Solution description:
1. User immediately get false as result of enter() if it is detected
that exit() has been run either before or concurrently. For that
purpose a new AtomicBoolean field prematureExit is introduced.
2. The exit() and enter() are reworked to be capable to run
concurrently and avoid EDT jam in the secondary loop. See comments to
the code for details.
***Testing:
Test launches the secondary loop upon a button press action triggered
by the Robot and simultaneously call exit() in a separate thread. The
Robot sends a big number of events (number is adjustable by ATTEMPTS
constant) to cover different concurrent states randomly. Along with
that the Robot sends a number of key events to ensure that UI is kept
responsive and all events are dispatched by EDT. The number of the
sent key events is tested to be equal to the number of processed events.
The test is run in different modes to test secondary loop launching
from EDT and non-EDT threads.
--Semyon