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

[Anton V. Tarasov]

Hi Mikhail,

This definetly looks better!

Trying to think of any possible pitfalls:

- EQ static initializer should not do anything connected with AppContext. Ok.
- There should not be any thread access issues with the EQ static fields. Ok, they are all final.
- Unsafe.getUnsafe() may throw SecurityException when its caller class is out of the system domain. 
Ok, AppContext is a system class.

So, I personally can't see anything bad about the fix.

Regards,
Anton.

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