22
\$\begingroup\$

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&);
};
\$\endgroup\$
6
  • \$\begingroup\$ 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. \$\endgroup\$ Nov 30, 2011 at 21:15
  • \$\begingroup\$ 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... \$\endgroup\$
    – Nim
    Nov 30, 2011 at 22:00
  • \$\begingroup\$ @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. \$\endgroup\$ Dec 1, 2011 at 0:04
  • 2
    \$\begingroup\$ @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. \$\endgroup\$ Dec 1, 2011 at 0:05
  • 1
    \$\begingroup\$ Once I was hoping that compilers would start to guarantee the thread safe initialisation for this pattern. They still haven't :( \$\endgroup\$
    – Sjoerd
    Dec 1, 2011 at 2:03

2 Answers 2

2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ I didn't realize you had a form of the URL that's easily memorized. :) Yay. \$\endgroup\$
    – GManNickG
    Nov 30, 2011 at 22:46
  • 3
    \$\begingroup\$ -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. \$\endgroup\$ Dec 1, 2011 at 0:07
  • 5
    \$\begingroup\$ @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. \$\endgroup\$
    – jalf
    Dec 1, 2011 at 8:04
  • \$\begingroup\$ @GMan: yeah, I got tired of googling it... ;) \$\endgroup\$
    – jalf
    Dec 1, 2011 at 8:04
  • 2
    \$\begingroup\$ @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. \$\endgroup\$
    – jalf
    Jan 28, 2013 at 8:22
2
\$\begingroup\$

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>
#include <boost/thread/mutex.hpp>

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.

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.