Second Code Review for WeakReference leak in the Logging API (6942989)

[Daniel D. Daugherty]

On 6/22/2010 2:34 PM, Alan Bateman wrote:
> Daniel D. Daugherty wrote:
>> :
>>
>> Here is the URL for the webrev:
>>
>>     http://cr.openjdk.java.net/~dcubed/6942989-webrev/1/
>>
>> Thanks, in advance, for any reviews.
> Sorry for the late reply. Using the reference queue is much better. 
> I've looked through the new webrev and the approach seems reasonable.

Thanks!


> One nit is that the style is perhaps a bit different to the existing 
> code. For example, the alignment of the declarations in LoggerWeakRef 
> is different to the enclosing class and the other classes in this 
> package, the new package-private methods are final whereas the 
> existing package-private methods aren't. Nothing wrong, just looks a 
> bit different.

I got the idea for the "final" stuff from Tony's paper. I also
don't think these are things that subclasses need to override.


> One question, in drainLoggerRefQueueBounded I'm curious about the 
> check for loggerRefQueue being null. Is that needed?

This check:

 522             if (loggerRefQueue == null) {
 523                 // haven't finished loading LogManager yet
 524                 break;
 525             }

Yup. Without that check we crash during LogManager.<clinit> if I
remember the name correctly. The whole LogManager/Logger relationship
relationship is fraught with such warts...

Thanks for the review!

Dan