RFR: 8010939 Deadlock in LogManager

[Jim Gish]

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.gish at oracle.com