Second Code Review for WeakReference leak in the Logging API (6942989)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jun 22 13:45:27 PDT 2010
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
More information about the serviceability-dev
mailing list