<AWT Dev> Review Request for 8065709: Deadlock in awt/logging apparently introduced by 8019623

[Anton V. Tarasov]

Hi Mikhail,

Ok, you removed the new lock at least. Looks better now (no more comments from me). Thank you!

Regards,
Anton.

On 20.01.2015 17:26, mikhail cherkasov wrote:
> Hi Anton,
>
> please check new version: 
> http://cr.openjdk.java.net/~mcherkas/8065709/webrev.01/src/share/classes/java/awt/EventQueue.java.udiff.html
> I applied changes advised by your in offline, the synchronization and second check for null was 
> removed.
> PlatformLogger.getLogger is already synchronized and can be treated as sync block and second check.
>
> Thanks,
> Mikhail.
>
> On 1/19/2015 10:38 PM, Anton Tarasov wrote:
>>
>> On 19/01/15 19:15, mikhail cherkasov wrote:
>>> On 1/19/2015 5:59 PM, Anton V. Tarasov wrote:
>>>> Hi Mikhail,
>>>>
>>>> It seems the problem is not limited to EventQueue only. Even if you solve it for EventQueue, 
>>>> the EventQueue ctor may cause a chain of calls where some other AWT class is first accessed and 
>>>> initialized. What if it contains the same getLogger() call in a static block?
>>> it can, but currently EventQueue doesn't do such things.
>>
>> I guess that it's nearly impossible to guarantee that AppContext ctor will never ever call 
>> anything containing uninitialized loggers.
>>
>>>>
>>>> If you agree with this, then there should be a generic solution for the deadlock.
>>>>
>>>> What comes to my mind is the following. The deadlock happens due to a reverse lock taking 
>>>> order. Can we change it?
>>>>
>>>> In LogManager.getUserContext() it seems like the javaAwtAccess lock scope can be narrowed down. 
>>>> Namely, we can move the javaAwtAccess.getAppletContext() line out of it.
>>> it's already done:
>>> http://hg.openjdk.java.net/jdk9/dev/jdk/rev/83f20d8bc13a
>>
>> Oh, thanks, I didn't read all the discussion the first time. Well, I see there's another deadlock 
>> involving the sun.util.logging.PlatformLogger class...
>>
>>>> In which case the method implementation should be additionally synchronized. For instance, we 
>>>> can use the getAppContextLock for the whole body of the method. Is the method implemented 
>>>> somewhere else except the AppContext class?
>>>>
>>>> Will it work from your point of view? (Probably, I didn't take into account all the details.)
>>> All this tricky synchronizations was done on purpose, I believe it was done for performance sake.
>>> So I want to make as less changes as possible, it's better to still miss couple cases then 
>>> introduce a
>>> new big issue in the last public update. Anyway, this fix plus 
>>> http://hg.openjdk.java.net/jdk9/dev/jdk/rev/83f20d8bc13a
>>> should cover all cases, the only possible if EQ ctor will lead to javaAwtAccess, but it doesn't.
>>
>> Ok, I'm fine with the fix (it looks harmless). However, I can't say I like it because it 
>> introduses new lock and breaks consistency (in all the other cases we get loggers directly)...
>>
>> I'd look for any better solution in an appropriate time slot.
>>
>> Reagards,
>> Anton.
>>
>>>>
>>>> Regards,
>>>> Anton.
>>>>
>>>> On 16.01.2015 17:18, mikhail cherkasov wrote:
>>>>> Hi all,
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8065709
>>>>> Webrev: http://cr.openjdk.java.net/~mcherkas/8065709/webrev.00/
>>>>>
>>>>> AppContext creation is guarder by getAppContextLockand, but during AppContext creation
>>>>> we also call EventQueue initialization, during EQ initialization logger initialization happens
>>>>> it acquires "javaAwtAccess".  if "javaAwtAccess" is acquired by other it can lead to deadlock:
>>>>>
>>>>>
>>>>> T1                                                   T2
>>>>> start AppContext  creation
>>>>> acquire javaAwtAccess
>>>>> acquire getAppContextLock
>>>>> init EQ                                         call getAppContext
>>>>> init Logger                                   waiting for getAppContextLock
>>>>> waiting for javaAwtAccess
>>>>>
>>>>>
>>>>> I applied the fix suggested in jbs comments by Petr.
>>>>> I replaced eager logger initialization in EQ with lazy, so we won't invoke Logger
>>>>> during EQ initialization as result no deadlock.
>>>>>
>>>>> Thanks,
>>>>> Mikhail.
>>>>>
>>>>
>>>
>>
>