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

[Daniel D. Daugherty]

On 6/10/2010 8:10 PM, David Holmes wrote:
> Daniel D. Daugherty said the following on 06/11/10 11:50:
>> On 6/10/2010 7:08 PM, David Holmes wrote:
>>> I wonder about the performance implications of doing this search 
>>> each time a logger is added? These types of cleanups are always a 
>>> fine line between minimizing cleanup up on the main path, and 
>>> ensuring too much garbage doesn't accumulate. I'm a little concerned 
>>> this is a lot of clean up code (potentially) on the main path.
>>
>> Agreed. In this particular case, I'm slamming down hard on the
>> correctness side. The theory is that there shouldn't be too many
>> Logger objects in the normal system and once you've added them,
>> then this fix doesn't come into play.
>
> So the original problem was exposed through excessive use of anonymous 
> loggers ... but now if those loggers are no longer referenced the next 
> anonymus logger created will trigger clean up of all such unreferenced 
> loggers (well assuming the weak-ref has been cleared), so the set of 
> children to traverse should be kept small.

The test case submitted by the customer used anonymous Loggers.
However, I believe the customer's original app used regular Loggers,
and they noticed the increasing WeakReference memory usage over a
long while. They created the anonymous Loggers test case as a way
of quickly reproducing a WeakReference leak. I have a query in about
their real usage pattern, but I haven't heard back.

One of leaking WeakReference code paths is common to both anonymous
and regular Loggers. So if something, e.g., an applet, is creating
massive numbers of anonymous Loggers, then creating either an
anonymous or regular Logger will cleanup up any stale WeakReferences
on the common code path.

The second leaking WeakReference code path is only used by regular
Loggers. So only the creation of a regular Logger can cleanup any
stale WeakReferences on that code path.

I think their app is also suffering from not saving a strong Logger
reference so the regular Loggers are being periodically GC'ed and
then recreated. A common (broken) usage of Logging is:

    Logger.getLogger("MyLogger").log("some message");

By never hanging onto a strong Logger reference, the Logger named
"MyLogger" will eventually be GC'ed and then recreated the next
time that it is used. See the following bug for gory details:

    6949710 3/3 the GC'able nature of Logging objects needs to be
                made brutally clear

When I finish writing the JavaDoc updates for that one, perhaps
you'll be kind enough to be one of my reviewers... :-)

> But I still wonder if there are apps out there that will create large 
> logger hierarchies ...

Maybe, but I'll deal with that as a performance fix rather than
an escalated memory leak.


>
>> Can I put you down as a "thumbs up"?
>
> Yes. :)

Thanks! And thanks for the thoughtful review!

Dan