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

[Semyon Sadetsky]

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