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

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Thu Jan 22 13:23:36 UTC 2015


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


-- 
Best regards, Sergey.



More information about the awt-dev mailing list