33

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;  
}     
Shadow The GPT Wizard
  • 66,030
  • 26
  • 140
  • 208
Atul
  • 501
  • 1
  • 5
  • 17
  • I am in a single threaded application – Atul Jan 02 '12 at 09:56
  • 1
    Why would you want to destroy a singleton? Allocate it statically. –  Jan 02 '12 at 10:05
  • 4
    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 singleton – jalf Jan 02 '12 at 10:05
  • Wouldn't it be a cause of memory leak ? I mean, I am doing a new of m_pInstance and not deleting it ? – Atul Jan 02 '12 at 10:07
  • @Atul don't use a pointer and it will not leak. Allocate it statically. Remove the asterisk. –  Jan 02 '12 at 10:08
  • 2
    @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 Jan 02 '12 at 10:17
  • 5
    @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 application – sdevikar Aug 24 '15 at 18:56

6 Answers6

44

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 !

Guy Avraham
  • 3,482
  • 3
  • 38
  • 50
Offirmo
  • 18,962
  • 12
  • 76
  • 97
  • 13
    "So think before using it. But go ahead anyway, that is the good way to learn !" This is excellent advice, the best way to find out why people advise against something is to do it anyway and come to the same conclusion. – John Neuhaus Jan 20 '15 at 16:01
24

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.

thiton
  • 35,651
  • 4
  • 70
  • 100
  • 1
    +1 for uniqueness issue. I have often seen people implementing a DB accessor as a singleton, and then they need a second DB... – Matthieu M. Jan 02 '12 at 16:26
14

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

Community
  • 1
  • 1
Paul Manta
  • 30,618
  • 31
  • 128
  • 208
  • If a tool has no justifiable or worthwhile uses, is it "evil"? Maybe not, but it's not something you should ever consider putting into your code either. Feel free to prove me wrong, and show me where a singleton can legitimately be used to solve a problem *better* than the non-singleton solution. – jalf Jan 02 '12 at 10:18
  • @jalf I'll just give examples of respectable libraries: Boost.Serialization and Boost.Python. – Paul Manta Jan 02 '12 at 10:22
  • That's not examples, that's just namedropping. Where do those libraries use singletons, and why is it a superior solution? Boost's solution isn't always by definition the best one. Otherwise every parser would be written with Boost.Spirit. ;) – jalf Jan 02 '12 at 10:24
  • @jalf I said examples of "libraries". Try to make something as easy to use yet as complex as Boost.Python without some kind of global registry. The singleton wrapper over the global registry just provides a nice and safe interface – Paul Manta Jan 02 '12 at 10:25
  • Yes, you said examples of libraries. But I **asked** for examples of singletons being used to solve problems, and I assumed you mentioned those libraries in an attempt to answer that question, which it didn't do. Now, is there any particular reason why this global registry can't be just that, a *global*? Is there any harm in allowing me to create a secondary registry (for testing, perhaps)? – jalf Jan 02 '12 at 10:25
  • @jalf The registry isn't part of the interface, it's an implementation detail; you, as the user, don't need to worry about it. The reason you shouldn't have as just a global is so you can better control how it is accessed. If you're going to need a global, why not wrap it such that it's usage is more predictable? – Paul Manta Jan 02 '12 at 10:31
  • and the library maintainer doesn't need unit tests either? Which, once again, would be possible if the registry could be instantiated at will (and a single global instance existed for convenience), instead of being a singleton. (Incidentally, how does a singleton "control how it is accessed"? – jalf Jan 02 '12 at 10:33
  • 1
    Global, global, access, global, access, blah. Not a single reason to justify the singularity. If you want to justify singletons you need to justify the whole package. – R. Martinho Fernandes Jan 02 '12 at 10:38
  • by the way, I'm currently working on a code base where singletons were used where it "felt appropriate". And the funny thing about singletons is that afterwards, when you have to maintain the code, they no longer feel appropriate *at all*. – jalf Jan 02 '12 at 10:44
  • @jalf You don't have to force the original class to be a singleton, but you can have a `class Singleton : public T` wrapper over the global instance you're using. The wrapper can control access in multithread programs, prevent accidental copying, etc. You can have a global `getInstance()` function, but that does not prevent copying. – Paul Manta Jan 02 '12 at 10:59
  • Why do you consider it an advantage to be able to "prevent copies from being made"? In my experience, it's something you never **need** in practice, but quite often, it turns out that even in cases where you'd never need copies, you end up being wrong. I'd say it's a premature constraint, making your code less flexible, for absolutely no gain – jalf Jan 02 '12 at 11:31
  • @jalf Prevent _accidental_ copies. you can design `Singleton` so that you can still create a copy of `T`, if you explicitly demand it. Preventing copies is like const-correctness: most of the time you don't need it, except that one time when you didn't pay attention and you did need it. – Paul Manta Jan 02 '12 at 11:34
  • It's not a singleton if it allows more than one instance to exist. So you're saying singletons are ok, because if you just write them to *not be singletons* then one of their weaknesses is negated? And even if you go around accidentally copying objects a lot, you can do this perfectly well with non-singletons as well. – jalf Jan 02 '12 at 11:50
  • er, you can prevent this perfectly well with non-singletons as well. Sorry if that was a bit unclear – jalf Jan 02 '12 at 11:58
  • I'm just saying that `Singleton<>` works well as an access controller. Making the original class noncopyable might be 'artificial', ie. conceptually it should be copyable, but you made it impossible to do so because of your specific usage. You can design `T` as it should be designed, and if you need a global instance you can wrap it in an access controller to ensure that no matter how the original class works, you can't misuse your global instance. – Paul Manta Jan 02 '12 at 12:09
  • 1
    What does it mean to "misuse the global instance"? As for the other point, it seems you're saying that the reason you like singletons is that you can extend them with a property that singletons do not normally have, but which you *can* add to any arbitrary type. In which case you do not like singletons, and you are not defending singletons. – jalf Jan 02 '12 at 12:18
  • @jalf I went over that: 'misusing' as in creating accidental copies, destroying it, etc. I guess it's an unfortunate name; what do you think I should call my access controller? `AccessController` doesn't really mean much (doesn't say how it actually controls access). – Paul Manta Jan 02 '12 at 12:31
  • `void f(T); f(Singleton::Instance());` <- accidental copies, just like with any variable. And I can't even think of a way to accidentally destroy a global `delete &global;` perhaps (it's undefined behavior)? But even then `delete &Singleton::Instance()`. – R. Martinho Fernandes Jan 02 '12 at 12:38
  • Ok, I no longer understand what you're trying to achieve. How exactly do your "access controllers", whatever we call them, prevent accidental copies? If the class itself is copyable, then nothing the wrapper can do is going to prevent copying. But apart from this, I'd say that `CopyController` would be a reasonable name for a class whose purpose it is to control copying of some class – jalf Jan 02 '12 at 12:43
  • No `Instance` function then? Because if it has no `Instance` function it doesn't sound like a singleton anymore. And if it has a `Instance` function that returns a `T&`... You can do pretty much anything on a `T&`. – R. Martinho Fernandes Jan 02 '12 at 12:47
  • @R.MartinhoFernandes Make it `static Singleton& Instance()`, `Singleton` inherits from `T`, its conversion operator has been made non-public, as have the constructors, destructor and assignment operators. – Paul Manta Jan 02 '12 at 12:58
  • @jalf Make the conversion `explicit operator T()` (C++11), and constructors, destructor and assignment operators non-public. – Paul Manta Jan 02 '12 at 13:03
  • Don't forget to change every function that takes a `T&` to take a `Singleton&` because you can't make the copy constructor nor the destructor of `T` private. (unless you go and change them directly, of course) – R. Martinho Fernandes Jan 02 '12 at 13:09
  • @R.MartinhoFernandes Why 'change'? Usually you can decide from the start if, for example, all your implementation code needs to use a global registry. – Paul Manta Jan 02 '12 at 13:25
  • Usually, the thing you decide from the start turns out to be wrong. And now you're in the highly contrived situation where your code must, any time you want to use a `T`, decide *in advance* whether to take a `T` or a `Singleton`, and if you change your mind, or even worse, want to write code that can be used with multiple different instances, then you're just out of luck – jalf Jan 02 '12 at 13:27
  • @Paul: Ok, and now we're back on testing, which is again impossible. Now that your code only takes `Singleton` you must use the singleton even in tests. – R. Martinho Fernandes Jan 02 '12 at 13:29
  • I have to say I've never run across that concept before, and it seems an incredibly contrived way of making implicit copying of copyable classes impossible, which in itself is a non-goal, because if a class is copyable, it means that it is safe to copy it. But if it retains the drawbacks of singletons (that all clients are forced to use the single global instance or nothing), then yes, it's a bad solution to a non-problem – jalf Jan 02 '12 at 13:34
  • @jalf I haven't really done a good job explaining my position. Try reading [Boost.Serialization's use for singletons](http://www.boost.org/doc/libs/1_37_0/libs/serialization/doc/singleton.html) and also look at [this code](http://www.boost.org/doc/libs/1_40_0/boost/serialization/export.hpp) (search for "BOOST_CLASS_EXPORT_GUID"). My entire point is that for very specific corner cases, singletons are justifiable. I don't consider any technique evil. – Paul Manta Jan 02 '12 at 13:53
  • But you haven't shown a "very specific corner case where singletons are justifiable". You've shown a needlessly contrived way of making it harder to copy a copyable class. But it is not a singletons, and says nothing about why singletons can be justifiable. And the "justification" for singletons still seems to boil down to "As far as I know, I'll never need a second instance of this class, so if I'm right, the singleton will cause me no harm. But if I'm wrong, it'll bring my entire code base crashing down". Which is a pretty anemic defense. "at best, singletons do no harm" – jalf Jan 02 '12 at 14:07
  • @jalf I don't think this discussion can ever end. – Paul Manta Jan 02 '12 at 14:11
  • I can see two easy ways in which it can end: you show me a case where a singleton *could not* simply be replaced by a global, where the "precisely one instance shall always exist" constraint is actually (1) necessary, and (2) beneficial. Or alternatively, if you'd admit that *a class which allows multiple instances to exist is not a singleton, and thus, what you've been defending isn't actually a singleton at all*. In the first case, we'd agree that singletons have their (rare) uses. In the latter case, we'd agree that if any such uses exist, we haven't found them yet – jalf Jan 02 '12 at 14:34
  • @jalf Whatever, if you don't call that a singleton, so be it. I guess we'll agree that they, whatever they are, have their rare uses, then. – Paul Manta Jan 02 '12 at 14:43
  • I didn't say that. You claim that your AccessController has its rare uses, and I have no particular opinion on that. But I claim that singletons (the ones that, as their name implies, enforce a *single* instance) do not have any legitimate uses, and you have been unable to come up with a counterexample. :) – jalf Jan 02 '12 at 14:54
  • @jalf I wasn't talking about singletons (as you define them) either. I was talking about "access controllers". **|** Oh, I get it. If I provide an example now we'll agree that they have their rare uses. If you want, see the links posted above; this discussion has already gotten pretty lengthy. – Paul Manta Jan 02 '12 at 15:04
  • @jalf: I use singletons a lot, f.i. for a Resource Manager (handling cached files in an application). I cannot see how you'd manage to do that efficiently WITHOUT a singleton. And there are many more uses for them. – Robert Jan 02 '12 at 15:11
  • @Robert: give it a try, then. You have two *obvious* alternatives. First, explicitly pass references to the places that need them, and second, use a *global* to provide *global* access, instead of using a *singleton. – jalf Jan 02 '12 at 16:03
  • Your *Singleton* class is wrong. The constructors and destructors should be `private`, otherwise I can implement a `DerivedSingleton` that inherits from `Singleton` and have a copy of your inner state... joice. Also, I personally consider the uniqueness of a `Singleton` to be an implementation detail and would recommend using a Monoid instead. – Matthieu M. Jan 02 '12 at 16:30
  • @jalf Here's a rather lengthy answer I posted on gamedev.se some time ago: http://gamedev.stackexchange.com/a/17759/6188. The solution I suggest there uses singletons as an implementation detail, in a way in which I think is entirely justifiable and probably can't really be done any other way. – Paul Manta Jan 02 '12 at 16:50
  • @jalf: There is no issue with access. The problem the singleton solves is data initialization/containment/concealing/destruction. Of course I could pass references to a resource manager everywhere, but it just complicates code with NO added benefit. And a global, how is it semantically better than a singleton ? A singleton IS a global. – Robert Jan 02 '12 at 17:09
  • @MatthieuM. I disagree about the monoids. Sometimes uniqueness should explicit, as is the case of a resource repository where the user should be reassured that he's using the same repository. whether you express the uniqueness through a singleton or through dependency injection is up to you. **|** I also disagree about private v. protected. You could design your singleton such that it allows inheritance. If you don't want to allow that, however, you are right, the constructors and destructors should be made private. – Paul Manta Jan 03 '12 at 07:46
  • Regarding the gamedev example: this is basically a plugin system that you are describing. The non-singleton solution simply implies to have a file in which you perform all registrations (explicitly) rather than relying on globals (constructed in an unknown order) to perform this registration for you. You use the Singleton because *you* find it convenient. I personally find it non-obvious and would rather have a single file where I can look-up in a single glance everything that is registered. It is definitely subjective (as all matters of style). – Matthieu M. Jan 03 '12 at 08:14
  • @MatthieuM. Indeed, I found it convenient to have the registration right next to the actual class rather than grepping a separate file to see if I registered it properly. I can understand if people don't like to do it like that, but I don't understand what is non-obvious about what's happening. At any rate, my point was that if you did want to make the registration possible to do next to the class, you'd need the singleton to do it properly. – Paul Manta Jan 03 '12 at 08:28
  • @MatthieuM. And they're not globals, they're class static private constants. – Paul Manta Jan 03 '12 at 08:33
  • What's the use-case for private unimplemented destructor in the above example? If the constructors are hidden, destruction is out of the picture, no? – DigitalEye Mar 11 '16 at 21:56
8

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.

wilx
  • 17,697
  • 6
  • 59
  • 114
  • It will be great if you can explain it by extending the given example ? – Atul Jan 02 '12 at 10:00
  • @wilx: or use `atexit`, it's been made for that. – Matthieu M. Jan 02 '12 at 16:31
  • Wow. What an answer, this answer should get -2000 or something... Why is your short answer "do not use singletons"? There are the places where you need to use "singleton" (rare, but still, they happen), otherwise, the design pattern wouldn't be invented... – Silidrone Oct 24 '17 at 12:06
  • 2
    @MuhamedCicak FYI patterns are not invented. They are discovered by observing existing codebases. It isn't rare that a decision made 20 years ago turns out to be a mistake today, and GoF book which popularized singleton pattern is from 1994. FTR one of the authors of that book is now saying a singleton pattern was a mistake. – milleniumbug Oct 24 '17 at 14:28
8

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();    
}  
Alessandro Pezzato
  • 8,603
  • 5
  • 45
  • 63
  • 1
    Don't do this!! If `exit()` is called during execution, the singleton will not be destroyed. – imreal Jan 16 '20 at 20:41
1

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>;
}
rich
  • 93
  • 1
  • 1
  • 10