I have created this class to simulate a thread-safe Singleton.

Have I missed anything?

#include <boost/thread/mutex.hpp>

class Singleton
{
public:
static Singleton& GetInstance()
{
boost::mutex::scoped_lock lock(m_mutex);
static Singleton instance;
return instance;
}

private:
static boost::mutex m_mutex;

Singleton() {}
~Singleton() {}

Singleton(const Singleton&);
const Singleton& operator=(const Singleton&);
};

• Maybe I'm missing something. Why not just declare your singleton where the mutex is declared? Then you wouldn't even need the mutex because you don't have a race for the constructor. Nov 30 '11 at 21:15
• Agree with Karl, the mutex is pointless, I don't believe there will be a race in the constructor of the static instance in the function...
– Nim
Nov 30 '11 at 22:00
• @Nim. That's the problem. In multi-threaded code the singelton may potentially be created by multiple threads that simultaneously call getInstance(). So yes it is valid. Dec 1 '11 at 0:04
• @KarlBielefeldt: The point of putting the singelton instance as a static member of a function is so the singelton is lazily created. If you put it with the mutex (at class scope) it will always be created. If the cost of creating the singelton is high you want lazy initialization. By putting the mutex in class scope it is guranteed to be created before main (before any threading starts and thus not substitutable to multi-threaded problems. Dec 1 '11 at 0:05
• Once I was hoping that compilers would start to guarantee the thread safe initialisation for this pattern. They still haven't :( Dec 1 '11 at 2:03

As usual, when dealing with singletons, consider that you probably shouldn't be making a singleton at all.

Read this, for example. A singleton is almost certainly not what you actually need, even if it is implemented correctly and in a thread-safe manner.

A plain old global might be a better option, or perhaps you should simply pass a reference around to those who need it.

• I didn't realize you had a form of the URL that's easily memorized. :) Yay. Nov 30 '11 at 22:46
• -1: Does not answer the question. He is not asking weather a singelton is good bad ideas. You could just as easily have posted this as a comment. Dec 1 '11 at 0:07
• @Loki: I looked in the FAQ for this site, and didn't see anything that discouraged answers like this. It says that if you are looking for "specific feedback about &lt;snip&gt; Best practices and design pattern usage in your code", this is the right side. And that "any aspect of the code posted is fair game for feedback and criticism." If I'm conducting a code review, the first thing I try to find out is "is this the right solution to the problem". I considered posting it as a comment, but to be honest, I think it's valid as an answer.
– jalf
Dec 1 '11 at 8:04
• @GMan: yeah, I got tired of googling it... ;)
– jalf
Dec 1 '11 at 8:04
• @l46kok huh? (1) it does address the question by suggesting other (better) ways to achieve the same, and (2) you do realize that this is on codereview.stackexchange.com, right, and not SO? I'm pretty sure a code review normally involves feedback, even feedback you didn't ask for.
– jalf
Jan 28 '13 at 8:22

Static m_mutex member isn't proper singleton itself. You can't use this singleton at static variables initialization time, because m_mutex isn't created lazily and may be not initialized.

Also, it's wrong to put actual logic to Singleton class, it should be template class.

#ifndef SINGLETON_H_689E0B11_E731_4555_AFE6_88598B582F1D
#define SINGLETON_H_689E0B11_E731_4555_AFE6_88598B582F1D

#include <cassert>

template<class T>
class Singleton
{
public:
static T& GetInstance()
{
assert(!is_destructed);
(void)is_destructed; // prevent removing is_destructed in Release configuration

boost::mutex::scoped_lock lock(GetMutex());
static T instance;
return instance;
}

private:
static bool is_destructed;

static boost::mutex& GetMutex()
{
static boost::mutex mutex;
return mutex;
}

Singleton() {}
~Singleton() { is_destructed = true; }
};

// force creating mutex before main() is called
template<class T>
bool Singleton<T>::is_destructed = (Singleton<T>::GetMutex(), false);

#endif // SINGLETON_H_689E0B11_E731_4555_AFE6_88598B582F1D


As it have private destructor, it's noncopyable - no need to declare copy ctor and copy assignment operator.

• @Abyx, I have problems to understand the line bool Singleton::is_destructed = (Singleton::GetMutex(), false);. Which syntax you used to build this assignment? Dec 1 '11 at 16:14
• @q0987, it's C++ syntax.
– Abyx
Dec 1 '11 at 18:08