RFR: 8010939 Deadlock in LogManager
jim.gish at oracle.com
Fri Apr 19 19:44:43 UTC 2013
I've incorporated your changes, run JPRT, and posted an updated webrev:
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:
> 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.
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.gish at oracle.com
More information about the core-libs-dev