Hi Semyon,
The new version looks fine to me, however it seems to have a flaw currently:
- Suppose "exit" is called when "enter" is executing on EDT, at line 181. In this case "afterExit"
won't be reset to false by "enter".
- Suppose, "exit" is called when "enter" is executing on non-dispatch thread, at line 263. Same
problem with "afterExit".
Is that corrent? If so, then I'd say that "afterExit" should be reset to false by "enter" when it is
awaken, either from an event pump or sleep.
Also, please put a space b/w "if" and brace at the line 177.
Thanks,
Anton.
On 30.03.2015 16:07, Semyon Sadetsky wrote:
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
