Hi Semyon,
Why we call wakeupEDT in the exit method after the fix unconditionally?
Please add a comment to the code to describe this briefly.
On 01.04.15 14:50, Anton V. Tarasov wrote:
Hi Semyon,
This looks much clearer now. I'm fine with it!
Thanks,
Anton.
On 01.04.2015 14:14, Semyon Sadetsky wrote:
Hi Anton,
the updated webrev:
http://cr.openjdk.java.net/~ssadetsky/6980209/webrev.02/
--Semyon
On 3/30/2015 6:29 PM, Anton V. Tarasov wrote:
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
--
Best regards, Sergey.
