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

[Anton V. Tarasov]

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