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

[Anton Tarasov]

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