Memory leaks when using a singleton

Question

I have a class in which I implement the singelton design pattern. I know some people don't think its a good idea, but it helps a lot,

Anyway - I have a memory leak and vlagrind points me to these lines:

_singleton = new Manager(); //Manager::instance() (Manager.cpp:18)

And

Manager::Manager() : _file(new ofstream), _tasks(new map<int, Task *>()),
        _idState(new map<int, int>()), _closing(false), _pending(false),
        _lock(new pthread_mutex_t), _endLock(new pthread_mutex_t),  _cond(new pthread_cond_t),
        _flushCond(new map<int, pthread_cond_t *>()), _attr(new pthread_attr_t) {
//The last line is line 25 in Manager::Manager

Now in Manager's destructor I can't explicitly delete it, because it creates a silly loop (as destructor will be called when deleting _singleton resulting in an infinite loop). How do I get rid of this leak? Thanks!

P.s. here is Valgrind's output:

==17823== 512 bytes in 1 blocks are definitely lost in loss record 2 of 2
==17823==    at 0x4C27297: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17823==    by 0x40151E: Manager::Manager() (Manager.cpp:25)
==17823==    by 0x4014DB: Manager::instance() (Manager.cpp:18)
==17823==    by 0x406475: initdevice(char*) (outputdevice.cpp:66)
==17823==    by 0x4061D5: main (driver.cpp:21)
==17823== 
==17823== LEAK SUMMARY:
==17823==    definitely lost: 512 bytes in 1 blocks
=    =17823==    indirectly lost: 0 bytes in 0 blocks
==17823==      possibly lost: 288 bytes in 1 blocks
==17823==    still reachable: 0 bytes in 0 blocks
==17823==         suppressed: 0 bytes in 0 blocks

Addition: here's the code where I create Manager:

Manager.h:
class Manager {
public:
    ~Manager();
    static Manager * instance();
private:
    Manager();
    static Manager * _singleton;
};

And the implementation:

Manager.cpp:
Manager * Manager::_singleton = 0;

Manager * Manager::instance() {
    if (!_singleton)
        _singleton = new Manager();
    return _singleton;
}

"Now in Manager's destructor I can't explicitly delete it, because it creates a silly loop." - what does that mean? — Oliver Charlesworth, May 13 '12 at 14:46
Don't do it with pointers and it works much better. http://stackoverflow.com/a/1008289/14065 It even becomes thread safe on gcc (see notes in linked question). Even if you use pointers you should never return a pointer you should return a reference from `instance()` otherwise the caller is unsure if they should delete the object and they never should. — Martin York, May 13 '12 at 15:01
possible duplicate of [C++ Singleton design pattern](http://stackoverflow.com/questions/1008019/c-singleton-design-pattern) — Martin York, May 13 '12 at 15:05

Sergey Kalinichenko · Accepted Answer · 2012-05-13T14:58:49.683

4

One common way of implementing singleton in C++ is making the instance a function-static std::unique_ptr<T> inside the instance getter, rather than a class-static variable. This ensures a call of destructor upon program's completion, and lets you create an instance that gets accessed polymorphically e.g. through a pointer to an abstract base class.

Scott Meyers provided a good discussion of this topic in his "More Effective C++" book.

edited May 13 '12 at 14:58

answered May 13 '12 at 14:50

Sergey Kalinichenko

714,442
84
1,110
1,523

I don't think that this is the problem as I call the destructor explicitly – yotamoo May 13 '12 at 15:00
@yotamoo You aren't using it in multithreaded environment, are you? Because if two threads try getting the instance concurrently, one of them will leak the `Manager`. – Sergey Kalinichenko May 13 '12 at 15:02
@dasblinkenlight While it is a correct concern, it would be very weird if he saw this reproduced consistently. – KillianDS May 13 '12 at 15:05
1. If it's in a unique_ptr it doesn't matter what scope it's in. It can be global and it will still release as the program terminates. 2. The Meyers singleton doesn't use a pointer, it uses a static variable and returns its address. – Edward Strange May 13 '12 at 17:14

score 2 · Answer 2 · answered May 13 '12 at 14:46

2

Make Manager a static object, and it's constructor and destructor will automatically be called. Or if you must allocate it with operator new, put it in a smart pointer (unique_ptr if you can, otherwise auto_ptr) such that it will be destroyed when the pointer is.

answered May 13 '12 at 14:46

Andy Ross

11,699
1
34
31

1

Good answer except that when you make the variable static then you may need to worry about global creation order if your singleton is used in the initialization of any globals. Thus a good place for it is probably in the "instance()" function as the Meyers singleton does. This guarantees order because the first function use forces initialization. – Edward Strange May 13 '12 at 17:17
Thus the "if you must allocate it with operator new" bit -- dependencies in creation order are isomorphic to constructor arguments. – Andy Ross May 13 '12 at 21:27

Memory leaks when using a singleton

2 Answers2

Linked