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

[Semyon Sadetsky]

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