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

[Sergey Bylokhov]

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.