RFR: 8010939 Deadlock in LogManager

[Mandy Chung]

Hi Jim,

Thanks for fixing this.

On 4/12/2013 1:11 PM, Jim Gish wrote:
> Please review: 
> http://cr.openjdk.java.net/~jgish/Bug8010939-LogManager-Deadlock/
>

The fix looks okay.  I would recommend to move the call to 
manager.drainLoggerRefQueueBounded() back to LogManager.addLogger which 
was originally intended and make the two separate synchronized methods 
called at the entry point as it's described in the comments of the 
drainLoggerRefQueueBounded.

You added a comment listing the bug ID.   Our convention does not 
reference the bug numbers in the source unless it's absolute critical 
information to capture to help cross-referencing.  I wouldn't want to 
see the bug numbers in the source that are fixed in the past 18 years :)

Good that you have a test to reproduce the deadlock.   A few comments:
The thread should be daemon threads so that the test will terminate 
right away if there is a deadlock.
L99: is this needed to reproduce the deadlock?  Logger.getLogger has 
already called LogManager.addLogger to register the logger.
L111: what exception is expected to be thrown and ignored by the test?
L137,144 you can call ThreadMXBean.isSynchronizerUsageSupported() to 
test if monitoring of ownable synchronizer is supported instead of catch 
UOE.
L154: you may just throw an exception and terminate the test.

Better to rename your test to some informative name that will give some 
idea what it does.  It should have @summary to give a summary of what it 
tests.  There are several deadlock tests in test/java/util/logging.  I 
like your framework and probably good to consolidate these deadlock 
tests but this is something for the future clean up - just to mention 
it.  The LoggingDeadlocks*.java tests have the comments how the deadlock 
occurs that I find useful. It'll be good to add similar information in 
your new test.  Some formatting nit: L97 a space after "i=0;" and an 
extra space is many places (e.g. L69-71, L128, 141, 156, etc: a space 
not needed in front of the first parameter after "(",   a space of the 
last parameter before ")" not needed - L127, 134, 152, etc.

thanks
Mandy