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

[Sergey Bylokhov]

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.