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

[mikhail cherkasov]

Ok, the "lazy" version will be pushed.

Thank you .

On 1/22/2015 4:23 PM, 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.
> Lazy initialization is better, moreover we have such requests already:
> https://bugs.openjdk.java.net/browse/JDK-6880089
>>
>> 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.
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>