Singleton C++ Class - How to avoid mem leak in multi threading

Hello,
got a C++ doubt..

normally Singleton class is written like
Sing.h
==========
class Sing{
private:
Sing();
static Sing* instance;
public:
static Sing *getInstance;
};

Sing.cpp
========

Code:

---

#include "Sing.h"
Sing::Sing()
{
}

Sing* Sing::instance = 0;

Sing* Sing::getInstance()
{
        if (instance == 0)
        {
                instance =  new Sing();
        }
        return instance;
}

In case of singleton, dest never gets called. So, mem allocated using new does not have corresponding delete. So, this can lead to mem leak.

Hence,  i heard this can be solved using

Sing* Sing::getInstance()
{
        static Sing instance;
        return &instance;
}

---

But this proc can be used only for single thread. not for multi.

Why so. Why cant it be used for multithreading??

-Regards,
molus

as written it is not thread safe. But you could make it thread safe by using critical section or a mutex to snychronize the threads in getInstance(). see artical How To Use Synchronization Classes

That's a bit over-simplistic, Ancient: it is still necessary to create/initialise the critical section or mutex before calling the getInstance() function. Which brings up the same problem: the critical section or mutex is (as far as the program is concerned) a singleton....

A better strategy is to explicitly create the object instance and the critical section to protect it before launching any threads. If no threads are launched, the program is single-threaded, so the initialisation (creating the object and whatever synchronises access to it) doesn't have to be protected.

One thing to remember is that protecting the process of creating the object does not protect accesses to it. For example, it does not protect calls to member functions if they are invoked simultaneously by multiple threads.

Well, my doubt was that why does the creation of static var in getInstance func() & returning it not be used for multi threading, In this way the destructor need not be called.
Whereas if i use the static member var, then i need to allocate using new(), but the destructor never gets called!


I do agree that locking & unlocking the critical section is required in multi threading.

Quote:

---

Originally Posted by grumpy

That's a bit over-simplistic, Ancient: it is still necessary to create/initialise the critical section or mutex before calling the getInstance() function. Which brings up the same problem: the critical section or mutex is (as far as the program is concerned) a singleton.....

---

well, my intent was not to spoon-feed his every action and is why I provided a link. Afterall, he needs to do some reasearch on his own.

And Yes, there are several other alternatives

Quote:

---

Originally Posted by molus

Well, my doubt was that why does the creation of static var in getInstance func() & returning it not be used for multi threading, In this way the destructor need not be called.

Whereas if i use the static member var, then i need to allocate using new(), but the destructor never gets called!

---

Both forms are actually unsafe in a multi-threaded environment.

The short reason is because the C and C++ standards say nothing about multithreading, and different compilers (like target operating systems) handle such things differently.

The longer reason is, as Dragon has said, is that if two threads attempt to call the getInstance() method, they can preempt each other. In your first form;

Code:

---

Sing* Sing::getInstance()
{
        if (instance == 0)
        {
                instance =  new Sing();
        }
        return instance;
}

---

let's assume that we have two threads calling this method concurrently. Because none of the operations are atomic, one possible sequence is;

1) Thread 1 enters the function and finds that instance == 0.
2) Thread 2 preempts thread 1, and finds that instance == 0
3) Thread 2 creates a new Sing(), and stores its address in instance.
4) Thread 1 preempts thread 2, and creates a new Sing() and stores it's address in instance. Hence any reference to the dynamically allocated Sing() from thread 2 is lost (i.e. the object is leaked)
5) Thread 2 preempts thread 1 and returns instance
6) Thread 1 preempts thread 2 and returns instance.

This is just one of many possibilities with two threads. There are a large number of variations, as none of the operations (eg comparing instance with zero, invoking operator new, invoking the constructor of Sing, assigning the address to instance, returning the value of instance, etc) are atomic: several of those operations can be interrupted (or preempted, if the operating system forces it to happen) by another thread at any time. And things get even more complex with more than two threads trying the same stuff.....

Unfortunately, the second form:

Code:

---

Sing* Sing::getInstance()
{
        static Sing instance;
        return &instance;
}

---

does not solve the problem either. The reason here is that the time the static variable is created is also compiler dependent. All the C++ standard requires is that instance will be constructed at some time before the first call of getInstance(). If you get lucky (and you do with some compilers) static variables are created at program startup. If you get unlucky (and you do with some compilers, and you definitely do if the static variable is within a dynamically loaded link library [eg a windows DLL]) static variables are created just before the first call to getInstance(), and the problems are like those of using operator new --- the creation process can be invoked on multiple threads, and preempting can occur, giving similar problems as your first example.

The long and short of it is that, when doing multithreading, you (the programmer) needs to exercise explicit control over any operation (creating, manipulating, destroying) to any objects that are accessed from multiple threads. One strategy is to only do certain operations (eg creating and destroying the object) when it is guaranteed only one thread is executing (eg before threads are launched, and after they are terminated), so it is not necessary to synchronise those operations. Another strategy is to ensure all operations are synchronised.

Quote:

---

Originally Posted by Ancient Dragon

well, my intent was not to spoon-feed his every action and is why I provided a link. Afterall, he needs to do some reasearch on his own.

---

Yeah, but the link does not provide the information needed either.

I'm all in favour of telling fibs when teaching, to ensure that someone learns something which works reliably, even if it isn't 100% accurate (eg some details are left out, but those omissions won't get people into trouble, and they can be corrected later).

But I don't believe in telling fibs which, if followed precisely, actually encourage someone towards practices that get them in trouble.

Unfortunately, in this case, the simple answer you gave can get a beginner into trouble, and they wouldn't know it for quite a while --- the information is out there, but not exactly easy to find.