destructor of a singleton class is called twice!

i have the code as below :

    class Singleton {
        private:
        int i;
        static bool stnflag;
        static Singleton* single;
        Singleton(int a)
        {
            i=a;
        }
        public:
        static Singleton* getinstance(int);
        void function();
        ~Singleton()
        {
          cout << "destructor is being called" << endl;
          stnflag=false;
        }
};

bool Singleton::stnflag=false;
Singleton* Singleton::single=NULL;
Singleton* Singleton::getinstance(int a)
{
        if(!stnflag)
        {
          single = new Singleton(a);
          stnflag=true;
          return single;
        }
        else
        {
                cout << "already single object created" << 开发者_开发百科endl;
                return single;
        }
}

void Singleton::function()
{

cout << "private member value in single ton class is :" << i << endl;
}

int main()
{
        Singleton *s1,*s2;
        s1=Singleton::getinstance(3);
        s1->function();
        s2=Singleton::getinstance(4);
        s2->function();
        delete s1;
        delete s2;
        return 0;
}

when delete s2 is called the destructor is called second time also!!but how it is possible? already the pointer of the object is deleted at delete s1 ri8..but i'm getting the print statement in the destructor for second delete s2 too..can anybody giveme the reason..but as per my assumption it shud through an error of double free pointer ri8!!

Well, don't call delete on it twice!

Calling delete on an already deleted object is undefined behaviour, not a guaranteed crash.

You should consider having getinstance() return a reference, rather than a pointer.

s1 and s2 point to the same object. You call delete twice on that object which is Undefined Behavior. Deleting an object doesn't really do any magic - it just marks the memory occupied by it to be free for reuse. So don't expect that if you have deleted it, it'll vanish or something...

The issue is Undefined Behaviour.

You're right that it's a double free, but the results of a double free are not guaranteed. Sometimes you will get a crash, or at other times it may appear to silently "work". The results of this sort of thing are unpredictable and depend on a whole host of factors that are external to your program.

Don't be misled by this: the double free is still wrong!

Jst as you use getInstance you should have a returnInstance and not call "delete" explicity as you don't call "new". And you will need a reference count that will increase when you call the getInstance;

e.g.

Siggleton::getInstance()
{
   instances++;
   ....
}

void Singleton::returninstance()
{
        instances--;
        if(instances == 0 && signle != null)
        {
            delete single;
            single = null;
            stnflag = false;
        }
}

The simplest way to use a singleton is to have a static instance of the class declared globally somewhere, and have the get_instance function return a pointer to that copy.

A simple improvement is to have a wrapper class containing a pointer to relavent class, which is only allocated when its first needed, but still cleaned up at the end of the program. But even this is usually more trouble than its worth -- it saves some start-up code, but is difficult to get right if the singleton may be accessed from multiple threads, and generally ends up needing some extra synchronisation locking code.

What you're trying to do is delete the singleton object when you no longer need it, which is exactly what a singleton usually DOESN'T need: you probably shouldn't need to delete it at all, let alone twice.

Calling delete on the handle you get from get_instance is always wrong, since there's no way for it to tell if the (same instance of) the singleton has been acquired again inbetween acquiring the version you're trying to delete, and the delete.

There are a couple of ways to handle this:

(1) Just make sure the instance is deleted at the end of the program. (Eg. use auto_ptr, or if you can, scoped_ptr to hold the instance variable instead of Singleton*), and don't do anything at all to the pointers you get from get_instance. This will not clean up the instance inbetween two successive bits of code which acquire the singleton, but that's almost always overkill anyway -- usually if you acquire a singleton you need it for the life of the program. If you REALLY REALLY want to do that, read on.

(2) Keep a reference count, so when each bit of code is finished with the pointer it got from get_instance, it calls a "release_instance" function, and when the count goes back down to zero, it should delete the object, and set the flag back to zero so the next call allocates it again.

(3) Automate #2 by using boost::weak_ptr and boost:shared_ptr. The internal pointer should be weak_ptr and get_instance should return shared_ptr. Each shared_ptr shares a reference count, counting how many shared_ptrs have been given out. When the last goes out of scope, the count goes to zero and the resource is deleted, and the weak_ptr becomes invalidated (but you can interrogate it to see if it's invalid). The code for get_instance should (a) check if the weak_ptr is valid, if so, convert it to a shared_ptr and return that (which increments the reference count) or (b) if not, allocate a new value in a shared_ptr, also store it in the weak_ptr, and return the new shared_ptr.

That is valid, but it's very very unusual you want to allocate only one copy, but want to bother deleting it between uses. If you do, a more normal way would be to have some submodule of your program which allocates a "Singleton" for the duration of that sub-module, and then returns handles to it, like in #1, but deletes it when the module shuts down. (Preferably checking all the handles are finsihed with first.)

What's specifically wrong with your example

Your code prevents Singleton being allocated more than once, and makes it (probably) unescessary to delete it more than once, but doesnt' actually prevent you deleting it more than once, which is what you did.

Both your s1 and s2 pointers point to the same object. You can call delete on it twice -- you could call delete on it three times if you wanted. But each time beyond the first, delete (a) deallocates some memory which is already deallocated and (b) calls the destructor on an empty section of memory, pretending it contains a singleton object.

(It's often good practice to set pointers to zero after deleting them, to avoid doing this by accident, but it doesnt' help in your case, because you have two different points to the same thing, so zeroing one of them still doesn't prevent delete being called on the other.)

What actually happens when this code runs? It calls delete. The compiler typically does one of two things (a) keeps a global list of all memory allocates with new, and since this memory wasn't allocated there any more, doesn't do anything for the delete (which may be a memory leak, but will normally not be a crash unless some intermediate code has allocated some new memory which was given the same address) or (b) doesn't know any better, doesn't do anything to free the memory, but calls the destructor on it. This may (i) do nothing, if the constructor doesn't access any memory of the class, or if the memory of the previously deallocated Singleton hasn't been overritten yet (ii) cause an access violation, if the destructor reads/writes to unallocated memory it thinks is in the class (iii) cause a horrible crash if it writes to memory allocated to a new object in the meantime. (ii) is the best, because you discover the eror. (iii) is worse because the error is hard to find (i) is worst, because the error may show up some unpredictable time later.

You could prevent this by making the deconstructor also private, which would remind you to call it only from inside the class, where you called the constructor from.