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

[Anton V. Tarasov]

On 22.01.2015 16:23, Sergey Bylokhov wrote:
> On 22.01.2015 13:42, mikhail cherkasov wrote:
>> 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/ ?
> No, no , no. I completely disagree on this solution. Besides that it not readable, it also 
> contains an  unobvious restrictions on EventQueue class.

As for me, this - Unsafe.getUnsafe().ensureClassInitialized(EventQueue.class) - is self-documenting, 
perfectly readable line of code. But Ok, let it be a matter of teste.

Yes it puts some constraints on EQ, but they look quite natural (I hope no one will think of putting 
threaded code into the EQ's static block).

> Lazy initialization is better, moreover we have such requests already:
> https://bugs.openjdk.java.net/browse/JDK-6880089

Well, I'm ok with having the loggers statically or lazyly initialized, but consistent accross the 
classes. (By the way, the latter approach, at least as it's suggested in 6880089, will bring an 
extra function call into every log message, regardless of whether the log is enabled or not.)

Regards,
Anton.

P.S. I don't insist on implementing the 3rd version of the fix. Leaving it at Mikhail's descretion. 
Thanks.

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