<AWT Dev> [9, 8] Review request for 8081485: EDT auto shutdown is broken in case of new event queue usage

[Sergey Bylokhov]

On 09.09.15 16:09, Anton Litvinov wrote:
> Hello Sergey,
>
> Thank you for review of the second version of this fix and for this
> correction. Before sending this review request I ran this regression
> test on OS X, Windows, Linux, Solaris platforms. The test executed
> successfully and was not blocked, probably because it does not use any
> JDK's private API.
>
> Will it be acceptable, if I just remove the following confusing "import"
> statements importing unused classes in the test instead of adding
> "@modules" tag?
>
> 31 import sun.awt.AWTAutoShutdown;
> 34 import java.util.EmptyStackException;

Yes, sure. Thanks for clarification.

>
> Thank you,
> Anton
>
> On 9/9/2015 3:24 PM, Sergey Bylokhov wrote:
>> Hi, Anton.
>> The fix looks fine, but after the fix was approved the modules came to
>> play. Please add correct @modules tag in the test before the push,
>> because it uses internal api from sun.awt package. I guess it should
>> be @modules java.desktop/sun.awt
>>
>>
>> On 03.09.15 18:39, Anton Litvinov wrote:
>>> Hello Sergey and Artem,
>>>
>>> After Anton Nashatyrev left the organization, I was assigned to this bug
>>> to complete work on it. Though Sergey approved the 1st version of the
>>> fix, the second approval was not received, and the fix was not
>>> integrated into "jdk9/client". Today I have compiled "jdk9/client" with
>>> the 1st version of the fix for all supported platforms and:
>>>
>>> 1. Verified using the manual test case from the bug description that
>>> *the fix resolves the bug*.
>>> 2. Found out that *the regression test "EventQueuePushAutoshutdown.sh"*
>>> from the fix *always fails on OS X* with the message:
>>>
>>>      "Unrecognized system!  Darwin"
>>>
>>> because the shell script expects only "SunOS", "Linux", "CYGWIN*",
>>> "Windows_*" versions of OS and does not execute the test on OS X.
>>> Therefore the 2nd version of the fix resolving this issue was created.
>>>
>>> Could you please review the 2nd version of the fix.
>>>
>>> Webrev (2nd version of the fix):
>>> http://cr.openjdk.java.net/~alitvinov/8081485/webrev.01
>>> Webrev (1st version of the fix):
>>> http://cr.openjdk.java.net/~anashaty/8081485/webrev.00
>>>
>>> The 2nd version of the fix modifies *only the regression test part* of
>>> the fix:
>>>
>>> Changes in
>>> "test/java/awt/Toolkit/AutoShutdown/EventQueuePush/EventQueuePushAutoshutdown.java":
>>>
>>> - "37     private volatile int status = 2;" - Modifier "volatile" was
>>> added, since the variable is accessed from several threads.
>>> - "28   @author     Anton Nashatyrev : area=toolkit" - This line was
>>> added.
>>>
>>> Changes in
>>> "test/java/awt/Toolkit/AutoShutdown/EventQueuePush/EventQueuePushAutoshutdown.sh":
>>>
>>> - "SunOS", "Linux", "CYGWIN*" were combined in one case of the switch
>>> and "Darwin" was added also to this case.
>>>
>>> Thank you,
>>> Anton
>>>
>>> On 7/2/2015 10:52 PM, Sergey Bylokhov wrote:
>>>> Hi, Anton.
>>>> The fix looks fine.
>>>>
>>>> On 01.07.15 16:07, Anton Nashatyrev wrote:
>>>>> Hello Artem, Sergey
>>>>>
>>>>>     thanks for your help with the issue investigation!
>>>>>
>>>>>     could you please review the following fix:
>>>>>
>>>>> fix: http://cr.openjdk.java.net/~anashaty/8081485/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Eanashaty/8081485/webrev.00/>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8081485
>>>>>
>>>>>     Problem: EventQueue.push() at the very beginning prevents the app
>>>>> from shutting down automatically
>>>>>
>>>>>     Reason: the EventQueue.push() first checks if the EDT for the top
>>>>> EventQeue exists (to transfer it to the new EventQueue) and then
>>>>> implicitly starts it.
>>>>>
>>>>>     Fix: do posting of dummy event only in case if the EventQueue had
>>>>> the EDT
>>>>>
>>>>> Thanks!
>>>>> Anton.
>>>>
>>>>
>>>> --
>>>> Best regards, Sergey.
>>>
>>
>>
>


-- 
Best regards, Sergey.