RFR: 8010939 Deadlock in LogManager
mandy.chung at oracle.com
Fri Apr 12 22:51:26 UTC 2013
Thanks for fixing this.
On 4/12/2013 1:11 PM, Jim Gish wrote:
> Please review:
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
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
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.
More information about the core-libs-dev