#139 Memory leaks?

[Martin Wimmer]

Dear All,

after making first tests using log4cpp I have to say that the functionality it offers is great, thanks for that!

However, when checking memory, I came across memory leaks, I did not find ways to get rid of.
Here is what I did:
(1) created a small sample based on http://log4cpp.sourceforge.net/#propfile
(2) included memory leak detection macros as offered by https://msdn.microsoft.com/library/x98tx3cf.aspx
(3) compiled it using Visual Studio 2015

Given the sample code below, please let me know if there is anything I missed concering clean-up strategy.
If not, please let me know, if/by when there would be a bug fix available to solve that.

thanks in advance, kind regards
Martin

Samle code is as follows (also attached to that request)
int main(int argc, char** argv)
{
//set debug flag for allocating memory leaks
_CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);

std::string initFileName = "log4cpp.properties";
log4cpp::PropertyConfigurator::configure(initFileName);

/
rest of the remaining sample code is not shown/used here
for the sake of brevity
/

log4cpp::Category::shutdown();

//report memory leaks
_CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_DEBUG);
_CrtDumpMemoryLeaks();

return 0;
}

memory leak trace (just excerpt, much longer in total) shows:
Dumping objects ->
{1144} normal block at 0x0139B3E8, 8 bytes long.
Data:

50 AE 39 01 00 00 00 00
{1143} normal block at 0x0139AE40, 48 bytes long.
Data: < 9 9 9 > E0 AD 39 01 C0 AC 39 01 E0 AD 39 01 00 00 CD CD
{1142} normal block at 0x0139B6F8, 8 bytes long.
Data: < 9 > 0C BC 39 01 00 00 00 00
{1141} normal block at 0x0138D6B0, 24 bytes long.
Data: < 8 8 8 > B0 D6 38 01 B0 D6 38 01 B0 D6 38 01 01 01 CD CD
{1140} normal block at 0x0139B6C0, 8 bytes long.
Data: < 9 > E8 BB 39 01 00 00 00 00
{1139} normal block at 0x01394B78, 20 bytes long.
.....
and many lines more

[Alexander Perepelkin]

Hi Martin,

Thank you for your investigation.
The question of memory leaks was issued earlier too.
Previous investigations (valgrind based as I remember) pointed out that small amounts of memory is not freed even after shutdown.
That is caused by singletons which allocate memory statically and do not release it until the exit. (As far as I remember it)
This is not kind of growing leak, which you can make sure of by looping your code and checking against the amount of reported memory. It must stay the same.
And by the way, will the debugger point to the variables which hold unreleased memory?

[Martin Wimmer]

Dear Alexander,
thanks for the prompt reply.

The debugger/macro would also point out where (file and line number) that came from - however, (to the best of my knowledge) not from included 3rd party libraries but only in own code.
We had the same issues with singletons in our code as well - that's why each of the singleton has its own destroy/release function and we have global manager making sure that any of these will be call prior to shutdown.
Are you planning to apply a similar strategy for handling singletons, as well?
As of today, I have to say, that this is causing issues on our end. We need to make sure that any allocated memory is released. If we accept a (hopefully stable) number of leaks, we would not be able to identify potential new issues when our development goes on (it is just too many of them in order to identify potential new issues in other code fragments).

thanks, kind regards
Martin

[Alexander Perepelkin]

Martin,

I think it is feasible. Will require quite a time being release candidate though.
Would you share your approach to the destroying singletons and global manager looking for them?
Do you say that debugger shows zero leak with this strategy?

[Martin Wimmer]

Dear Alexander,

yes, when using such kind of clean-up strategy, the debugger does not report and leaks.

Here is the general approach:
(1) Sample singleton class would look as follows

Header:

class Singleton
{
private:
// singleton instance
static Singleton* instance;

 // priv constructor
 Singleton();

public:
static Singleton* getInstance();
// public destructor
~Singleton();

 // clean up/release method
 static void destroy();

}

CPP:

// well, basically looks as a typical Singleton implementation with
Singleton* Singleton::instance=NULL;
// constructors, getters and the like - I omit this here
// destroy looks as follows:
void Singleton::destroy()
{
// of course class specific stuff like flushing streams etc.
// i.e. anything needed prior to a safe and clean exit
if (instance != NULL)
delete instance;
instance = NULL;
}

(2) a global SingletonManager or the like offering a close() or shutdown() routine, e.g.
void SingletonManager::shutdown()
{
// foreach singleton in your library
Singleton::destroy();
// also a nice registration approach (i.e., all singletons have to get registered at the Manager)
// could be chosen, here
}

(3) finally, everyone using your library could operate as usual, with the only amendment to call the shutdown() in the end, e.g., our code above could be as follows:

int main(int argc, char** argv)
{
//set debug flag for allocating memory leaks
_CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);

std::string initFileName = "log4cpp.properties";
log4cpp::PropertyConfigurator::configure(initFileName);

/
rest of the remaining sample code is not shown/used here
for the sake of brevity
/

// -> final and last step:
log4cpp::SingletonManager::shutdown();

//report memory leaks
_CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_DEBUG);
_CrtDumpMemoryLeaks();

return 0;
}

hope that helps. Please don't hesitate to contact me in case of any questions or concerns.
Would be great if you could let me know if you tend to implement this in future releases and - in case yes - when that might be available, approximately.

That would be really great. Cause - just to repeat what I wrote in my initial request - the functionality of log4cpp is really well provided and a lot of features/configuration options are given. Thanks for that!!!

Thanks, kind regards
Martin

[Alexander Perepelkin]

Martin,

I see the way you prefer.
Singletons are terrible piece of fortune although.
I've refreshed singleton-related troubles at http://stackoverflow.com/questions/1008019/c-singleton-design-pattern/1008289#1008289

Destruction case is not quite obvoius here. Suppose that we enhanced shutdown() with releasing our singletons.
We can not say that all our users do shutdown() safely. Those who do not do it at all, won't get hurt when they just update the library.
Those who call it now, may run into troubles with the new strategy releasing the memory.
Thread safety is an issue. If shutdown() was called not as the the very last move of the program, other alive threads would corrupt heap if they decided to log something at the end. I would count this bigger problem than a small leak.
Another probable issue is that user's singletons might make use of log4cpp's loggers. In their destructors they might still expect the logger's ecosystem to be still available. Not very solid approach, but if we break those apps, that'd be very disappointing.
If you see the way to work this around, it'll be great.

Not directly related to the log4cpp, but only your SingletonManager: do you respect dependencies between your singletons? Won't one of them try to use another one which was destructed already?

[Martin Wimmer]

Dear Alexander,

basically, our singletons' destructors work exactly as noted by Reed Copsey in the thread you mentioned in your last mail: we always set the singleton back to an initial state. That is, if someone is calling them again after destroy() was called, it would have to re-initiate itself. Of course the effect of cleaning memory would then be lost ...

Concerning our Manager: yes we do take care of dependencies and shut down singletons in an order that source singletons are released in the end (e.g. common resources like logging).

I see your concerns. Nevertheless I would opt for a possibility to clean up instead of just relying on the overall program termination. Just like the start of a program, one should also be concerned with its reliable shutdown. That would also mean: I would destroy singletons in the inverse order (like when using brackets in code) and release log4cpp at the end (after all of my other singletons, threads etc. have been terminated). That means: it would be the programmer's responsiblity to use log4cpp's shutdown the right way. I would do it that way, cause I would also assume that this shutdown ensures a correct order of releases and also clean final state (e.g., all messages written to disk).

kind regards
Martin

[Alexander Perepelkin]

Martin,

Would elaborate on how do you track dependencies in rhe manager? Is it being achieved automatically or manually?
Regarding releasing the new version - if we find out the safe path for freeing resources and not hurting existing users, that will be released as candidate in near weeks or month. I hope to find time for that.

[Martin Wimmer]

Dear Alexander,

currently, freeing the singletons is done in a predefined sequence. Basically, our program has well defined start-up and shut-down phases. We more or less can terminate the singletons in the inverse order.
We have identified some (but not many) dependencies between those. That is basically what needs to be taken into account. Apart from that the instances could be released in an arbitrary order.
Is it a large number of singletons you are confronted with and would it be rather easy or quite tricky (and risky) to build up such a dependency graph for log4cpp?

kind regards
Martin

[Alexander Perepelkin]

Martin,

Thanks for the answer.
Let me devote some time looking what can be done there.
Please feel free to raise this thread up if I do not respond in a week or so.

[Martin Wimmer]

Dear Alexander,

hope all is well. Do you have any news on that topic for me.
As mentioned, would be great if it is planned to change the memory cleanup during program shutdown so that memory leaks can well be identified in surrounding code.

Thanks, kind regards
Martin

[Alexander Perepelkin]

Martin,

I've made some investigations and found out that appenders were not cleared well at least.
Glad that you are still interested. I will get back to the topic then. Few code is pushed in the repository already.

Alexander.

[Martin Wimmer]

Dear Alexander,

thanks for your feedback - let me know when you plan a new release (also ones were that topic is partially addressed) so we can test it at an early stage.

kind regards
Martin

[Alexander Perepelkin]

Martin,

The exact date of release is the thing I can not really tell you, since I do that in my spare time.
It looks like I managed to resolve mem leaks with adopting nifty counter pattern for the appenders being created in the library.
This pattern involves using statically allocated memory, and creating singleton object in that area.
I created a test which showed absence of memory leaks with such approach.
The memory is fully freed after the main() returns since the object is statically allocated.
This is why if you insert call to CrtDumpMemoryLeaks() as the last statement in the main(), it will still report few unloaded blocks at this point in time.
But if you can intentionally omit CrtDumpMemoryLeaks(), then the report will be printed after deallocation of static data, and no unloaded blocks will be there. Prior to this solution singleton object was not cleaning its content, and report would still show leaked blocks.

I must note just for the case if you compile the library with CRTDBG Leak Detector that the solution needs to use placement new operator. CRTDBG forces us to redefine new operator and use their overloaded new if we want to see filenames and lines. By that the call to placement new gets mangled. That can be avoided by resorting back to standard new when placement new must be invoked. http://stackoverflow.com/questions/4990880/replacing-new-with-macro-conflicts-with-placement-new
Once again, this note is applicable only if you decided to recompile the library itself and redefine its own calls to new as guided by https://msdn.microsoft.com/library/x98tx3cf.aspx

If you are ok with such solution and see it useful for your purposes, I may clean up the code and publish it.

Alexander.

[Martin Wimmer]

Dear Alexander,

thanks for your feedback.
Yes, the proposed solutions sounds good and very helpful for us.
If you could provide us the sources the way you mentioned in your last post, that would be great.
We will test it as proposed to integrate the new build in our code and give you feedback.

thanks in advance and kind regards
Martin

[Alexander Perepelkin]

Martin,

I am going to push the code into separate branch. You can check out that branch, build and check the version with appenders being released. I'll put a note here when it's done.

Alexander.

[Alexander Perepelkin]

Martin,

I merged code into master branch and put new release candidate into downloads:
log4cpp-1.1.2rc2.tar.gz

Please try it out.
FYI: I added MSVC project msvc10\log4cppRunnable to check for memory leaks easier.
Debug configuration contains macro to perform memory leak checks.
There is new method Category::shutdownForced() in addition to previous Category::shutdown()
Category::shutdownForced() releases appenders, you need to invoke it if you want to make more than single configuration. msvc10\testPropConfig shows how.

Alexander.