30
votes

I was implementing a singleton pattern.Here,I am creating a new instance of Singleton* in GetInstance, when I try and delete it in the destructor, it does in infinite loop. How to avoid memory leak in this case ?

Please refer the below piece of code:

#define NULL 0
class Singleton  
{ 
    private :  
        static Singleton* m_pInstance;  
        Singleton(){};  

    public :

    static Singleton* GetInstance()
    {
        if(m_pInstance == NULL)
        {
            m_pInstance  = new Singleton();         
        }
        return m_pInstance;
    }

    ~Singleton()
    { 
        //delete m_pInstance; // The system goes in infinate loop here if i uncomment this  
        m_pInstance = NULL;
    }
};

Singleton*  Singleton ::m_pInstance = NULL;   

int main()  
{
    Singleton* pInstance = Singleton::GetInstance();
    delete pInstance;  
}     
6
I am in a single threaded applicationAtul
Why would you want to destroy a singleton? Allocate it statically.user142019
The entire point in a singleton is that you can't delete it. It is supposed to represent an object that is always accessible. If it isn't, then it certainly shouldn't be a singletonjalf
@Atul Why are you defining NULL 0? That's already offered by the language. Btw, if your compiler supports C++11, you should use nullptr instead.Paul Manta
@jalf "It is supposed to represent an object that is always accessible. If it isn't, then it certainly shouldn't be a singleton"... I am sorry, I beg to defer. That's not at all the "entire point" of singleton. It may be a side effect at most. The point of singleton, as the name suggests, is the prevention of multiple object creation of that class by your applicationsdevikar

6 Answers

41
votes

Of course it causes an infinite loop !

You call the destructor, but the destructor also calls the destructor, so the destructor calls the destructor again... and again...

If you want to use delete, you must use it from outside of the destructor and NOT call it again in the destructor.

To do that, you can use another static method which will mirror the GetInstance() method :

class Singleton  
{ 
public :

   ...

   // this method is a mirror of GetInstance
   static void ResetInstance()
   {
      delete m_pInstance; // REM : it works even if the pointer is NULL (does nothing then)
      m_pInstance = NULL; // so GetInstance will still work.
   }

   ...

   ~Singleton()
   { 
       // do destructor stuff : free allocated resources if any.
       ...
   }

Note : the other people warn you about using a singleton and they are right because this pattern is often misused. So think before using it. But go ahead anyway, that is the good way to learn !

22
votes

While the best practice is not using the singleton pattern in most cases, it is good practice to use static local variables in functions to create singletons:

static Singleton& Singleton::GetInstance() {
     static Singleton the_singleton;
     return the_singleton; 
}

To give some rationale to the best practice: Singleton-nage is usually unnecessary unless you have to represent a truly global resource. Singletons suffer from all the drawbacks of global variables (because they are global variables with some OO icing), and often have little justification in being truly singular. The naive programmer might want to implement God as a singleton object. The wise programmer doesn't and rejoices when the customer turns out to be a polytheist.

13
votes

Here's a more correct implementation of singletons:

class Singleton
{
  public:
    static Singleton& Instance()
    {
        static Singleton inst;
        return inst;
    }

  protected:
    Singleton(); // Prevent construction
    Singleton(const Singleton&); // Prevent construction by copying
    Singleton& operator=(const Singleton&); // Prevent assignment
    ~Singleton(); // Prevent unwanted destruction
};

The static instance gets created the first time you call Instance() and destroyed when the program closes.

But beware about using singletons. They are not evil, as some here think the are (I find that position irrational), but they are very easy to misuse and hard to use correctly. As a rule of thumb, don't use singletons for your "interface classes" (the ones that are used by other parts of the program); try to use singletons only as implementation details and only when it feels appropriate.


Edit: A usage example

Some time ago I posted an answer on gamedev.stackexchange and the solution I proposed made use of singletons as part of the implementation, not the interface. The code is commented and explains why singletons are needed: https://gamedev.stackexchange.com/a/17759/6188

8
votes

Add a static member Singleton::DestroyInstance() that delete the instance and call it from the main.

void Singleton::DestroyInstance() {
    delete m_pInstance;
    m_pInstance = 0;
}

/* ...................... */

int main()  
{
    Singleton* pInstance = Singleton::GetInstance();
    /* ... */
    Singleton::DestroyInstance();    
}  
7
votes

Short answer, do not use singletons.

Longer answer, never call delete on the singleton pointer in main(). Use some sort of static object that will delete the singleton when other global variables dtors are being called.

0
votes

using SingletonHolder of Loki-Library written by Andrei Alexandrescu.

#include "SingletonHolder"

class Singleton
{
//not allowed ctor
private:
   Singleton()
   {}

   ~Singleton()
   {}

   ...

   //Singelton on heap
   friend struct Loki::CreateUsingNew<Singleton>;
}

Singleton& get_singleton_pointer()
{
   return Loki::SingltonHolder<Singleton>::Instance();
}

In this example the Singlton will be deleted while programm was ended. There is also some other stategie to create singleton pointer using malloc, static... more detail take a look of: http://loki-lib.sourceforge.net/html/a00628.html

Alternativ you can create singleton just using a static variable:

template <typename T>
struct CreateUsingStatic
{
   static T& get_T_singleton()
   {
      static T t;
      return t;
   }
};

class Singleton
{
   ...
private:
   friend struct CreateUsingStatic<Singleton>;
}