<AWT Dev> [8] Review request for 8019623: Lack of synchronization in AppContext.getAppContext()

[Anthony Petrov]

Quite unusual to start a name of a class (or a variable) with a verb 
("GetAppContextLock"), but otherwise the fix looks fine to me.

--
best regards,
Anthony

On 10/09/2013 04:32 PM, Leonid Romanov wrote:
> Ok, here an updated webrev then, with the test added:
>
> http://cr.openjdk.java.net/~leonidr/8019623/webrev.01/
>
> On Oct 8, 2013, at 7:43 PM, Artem Ananiev <artem.ananiev at oracle.com> wrote:
>
>>
>> On 10/8/2013 6:54 PM, Leonid Romanov wrote:
>>> Hi,
>>> I tried different variations of your suggestion and none of them worked. The reason is timing: suppose two threads, A and B, entered AppContetx.getAppContext() and then AppContext.initMainAppContext() at the same time. We need thread A to be able to initialize mainAppContext, return from initMainAppContext() and then initialize local variable with AppContext to be returned without thread B intervening in between. This just doesn't happen.
>>
>> This is perfectly expected, that the created regression test won't fail without your fix. It will still be useful to prevent regression in the future.
>>
>> Thanks,
>>
>> Artem
>>
>>> On Oct 4, 2013, at 1:06 PM, Artem Ananiev <artem.ananiev at oracle.com> wrote:
>>>
>>>> Hi, Leonid,
>>>>
>>>> the fix looks fine.
>>>>
>>>> I believe it is possible to create a regression test for this fix. The test should create several thread groups and call AC.getAC() there, then check that all the returned AC's are equal. Although it won't 100% fail before your fix, it should 100% pass after it. What do you think about it?
>>>>
>>>> Thanks,
>>>>
>>>> Artem
>>>>
>>>> On 10/2/2013 6:22 PM, Leonid Romanov wrote:
>>>>> Hello,
>>>>> Please, review a fix for 8019623: Lack of synchronization in AppContext.getAppContext(). I don't think it makes sense to ensure that AppContext.getAppContext()/SunToolkit.createNewAppContext() is thread safe, because in practice we haven't had any problems with this combination. What we have had problems with is the getAppContext()/getAppContext() combination because of the double mainAppContext initialization.  Therefore, the fix takes care of this issue. While I could have simply made the whole AppContext.getAppContext() method synchronized, I wanted the most common path through it (returning cached AppContext) to remain lock-free, so I decided to place the lock around the most critical part, where initMainAppContext() gets called.
>>>>> As for a regression test, I couldn't find a way to detect double initMainAppContext initialization. We put newly created AppContexts into a thread-safe map, so whatever AppContext wins the race remains in the map, and the other is simply lost, without any convenient means to detect its existence.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8019623
>>>>> Webrev: http://cr.openjdk.java.net/~leonidr/8019623/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> Leonid.
>>>>>
>>>
>