Looks fine.
On 23.04.15 15:07, Anton V. Tarasov wrote:
I'm ok with the change.
Anton.
On 21.04.2015 13:12, Semyon Sadetsky wrote:
Hi Sergey,
Actually it's not needed and I've removed it for the no enter() brunch.
http://cr.openjdk.java.net/~ssadetsky/6980209/webrev.03/
--Semyon
the exit() is called and it detects that there was no enter() after
that scheduler
On 4/20/2015 2:17 AM, Sergey Bylokhov wrote:
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.
