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.
