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

[Anton V. Tarasov]

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