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

[Anton V. Tarasov]

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