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

[Anton Litvinov]

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;

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