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

Thu Apr 23 12:55:51 UTC 2015

Looks fine.

On 23.04.15 15:07, Anton V. Tarasov wrote:
> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

-- 
Best regards, Sergey.