<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