<AWT Dev> [9] Review Request JDK-6980209: Make tracking SecondaryLoop.enter/exit methods easier

[Semyon Sadetsky]

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
>>
>