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

[mikhail cherkasov]

Hi Anton, Sergey,

Anton advised today to implement eager initialization of  EQ's logger 
instead of lazy initialization of EQ's logger.
So now it will be initialized out of any AppContext's lock and I thinks 
it's definitely better solution because
no ugly getLogger methods, less changes and now we know when and where 
logger will be initialized.

Could you please review a new version:
http://cr.openjdk.java.net/~mcherkas/8065709/webrev.03/ ?

Thanks,
Mikhail.


On 1/20/2015 8:14 PM, Sergey Bylokhov wrote:
> On 20.01.2015 19:08, Anton V. Tarasov wrote:
>> Hi Mikhail,
>>
>> Ok, you removed the new lock at least. Looks better now (no more 
>> comments from me). Thank you!
> Personally I prefered to remove this logging, but this version looks 
> fine also.
>>
>> 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.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>