[Log4cpp-devel] log4cpp shutdown and memory leaks

[D. L. <lo...@no...>]

Hi,

I did some work on the shutdown/memory leak issue. My problem was, that
after using log4cpp and the PropertyConfigurator I have lots of small
memory leaks during application shutdown. Calling Category::shutdown
will not help in this issue. I did some investigation and came up with
the following solution:

1.) The PropertyConfigurator allocates all declared appenders in the
configuration file, but will not transfer ownership to one of the
categories. This results in the appenders not beeing deleted during
shutdown, since the Categories have not set the _ownsAppender flag to true.

There are two solutions to the problem:
a.) transfer the ownership of the appender to at least one Category or
introduce a dummy Category with the ownership of all instanciated appenders.

b.) call Appender::_deleteAllAppenders somewhere during shutdown.
This yields the question to whom the AppenderMap belongs, and who is
responsible for deleting the appenders. one solution would be to do
the deletion via a static variable within the Appender class, but this 
results in a circular dependancy with the HierarchyMaintainer. The 
HierarchyMaintainer exist earlier than the AppenderMap, and therefor 
will be deleted later. Within the destructor of the HierarchyMaintainer 
the deleteAllCategories function is called where all Categories and the 
contained appenders will be deleted. Deleting the AppenderMap before 
this call will result in a crash during destruction. One could force the 
creation of the AppenderMap before the creation of the 
HierarchyMaintainer, but this is also not good programming practice.

I solved the problem right now via a explicit call to deleteAllAppenders 
during the shutdown of the HierarchyMaintainer.



2.) deletion of the AppenderMap itself
Don't know how to that correct, yet. I added a delete call for
Appender::_allAppenders into the Appender::_deleteAllAppenders ()
function, since the map isn't used after a call to this function.

This should be better done via a specifc AppenderMap class, that can 
handle its contents and the map itself. And again, we must force this 
AppenderMap to exist before the HierarchyMaintainer, since we otherwise 
get shutdown oder dependancy.


3.) deletion of the NDC
The ThreadLocalDataHolder should reset its content during destruction to
delete the stored data object

4.) delete the HierarchyMaintainer.
This is a curcial part, since the HierarchyMaintainer should delete all
Categories during destruction. But I use a lot of static references to
Categories within my application, that will be invalidated in a wrong
shutdown sequence. The problem is, that there is no guarantee in wich
order static variables are constructed. So simply adding a shutdown
class to the HierarchyMainainter will not solve the problem, since an
other static Category& variable could have been initialized before the
shutdown member was initialized.

First I thought to introduce the a LogManager (like in Log4j) but this
doesn't solve the problem, only shifts it away from the
HierarchyMaintainer. The I realized, that I must add an implicit order
into the atexit sequence to install an atexit handler in the moment the
first static Category& is iniitialized. This can be done with an
auto_ptr in the HierarchyMaintainer like so:

     HierarchyMaintainer* HierarchyMaintainer::_defaultMaintainer = NULL;

     HierarchyMaintainer& HierarchyMaintainer::getDefaultMaintainer() {

         if (!_defaultMaintainer) {
            static std::auto_ptr<HierarchyMaintainer> maintainer (new
HierarchyMaintainer());
           _defaultMaintainer = maintainer.get ();
	}

         return *_defaultMaintainer;
     }

By stuffing the allocated HierarchyMaintainer into a static auto_ptr
within the getDefaultMaintainer function we will hook into the atexit
queue in the moment, the first time this function is called, and that is
exactly one element before the first static Category& that depends on
the HierarchMaintainer.

With this the HierarchyMaintainer will be deleted during application
shutdown, and all resources are freed.

On little problem still exists: What if after the destruction of the
HierarchyMaintainer someone tries to get a reference to a Category. This
again will fire up the HierarchyMaintainer resulting in the allocation
of objects.

The solution to this is not to use local references to Categories, only
static references.

A second solution would be to use the LogManager/LoggerRepository class
from log4j, that introduces a variable repository for Logger objects.
The default repository would be the HierarchyMaintainer. All calls to
getCategory/getLogger are redirected through the LogManager. After
shutdown one could return a static "do nothing" category from the
LogManager. Then, if you call the LogManager::getLogger("whatever")
after shutdown of the log4cpp library you get a valid logger, but this
logger will not do anything and all references to static Categories are
still valid until the HierarchyMaintainer is deleted.


-----

As a second solution I would like to say, that the Log4cppCleanup class 
wasn't that bad at all. I think now, that it was was simply wrong 
allocated. If you would do it with the auto_ptr method as described 
above and make a call to the Log4cppCleanup::getInstance function from 
all allocations of static variables it would have worked.

static Log4cppCleanup::instance* = NULL;

Log4cppCleanup::getInstance ()
{
   if (!instance)
   {
     // the auto_ptr will force the deletion of the Log4cppCleanup
     // after all depending objects have been deleted
     static std::auto_ptr <Log4cppCleanup> autoInstace (new Log4cppCleanup);

     instance = autoInstace.get ();
   }
   return instance;
}

HierarchyMaintainer& HierarchyMaintainer::getDefaultMaintainer()
{
   if (!_defaultMaintainer)
   {
     // force the Cleanup class into the atExit queue
     Log4cppCleanup::getInstance ();

     // don't need the auto_ptr here, since all cleanup is done
     // via Log4ccpCleanup
     _defaultMaintainer = new HierarchyMaintainer();
   }
   return *_defaultMaintainer;
}

----------


I attach a patch to the CVS checkout from 14.04.2003 to implement all
that stuff, except the proper handling of local categories during
shutdown returned from LogManager::getLogger ().

The patch includes also a change for the Appender::getAllAppenders to 
return a vector<Appender*>* and an additional method for the 
FileAppender to return its name. An last but not least I included a file 
named dontdiff to use for building diffs, to exclude unwanted files in 
the diff:

 > diff -Naur -X log4cpp/dontdiff ...

Perhaps you can give me some feedback about the two proposed solutions.

Thanks
Dirk