Hi Mandy,
I've incorporated your changes, run JPRT, and posted an updated webrev:
http://cr.openjdk.java.net/~jgish/Bug8010939-LogManager-Deadlock.1/
<http://cr.openjdk.java.net/%7Ejgish/Bug8010939-LogManager-Deadlock.1/>
Thanks,
Jim
On 04/12/2013 06:51 PM, Mandy Chung wrote:
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
--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com
