Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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&);
};
share|improve this question

migrated from stackoverflow.com Nov 30 '11 at 21:05

This question came from our site for professional and enthusiast programmers.

    
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. –  Karl Bielefeldt 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. –  Loki Astari Dec 1 '11 at 0:04
2  
@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. –  Loki Astari 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 :( –  Sjoerd Dec 1 '11 at 2:03

2 Answers 2

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.

share|improve this answer
    
I didn't realize you had a form of the URL that's easily memorized. :) Yay. –  GManNickG Nov 30 '11 at 22:46
1  
-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. –  Loki Astari Dec 1 '11 at 0:07
5  
@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
1  
@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>
#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.

share|improve this answer
    
@Abyx, I have problems to understand the line bool Singleton::is_destructed = (Singleton::GetMutex(), false);. Which syntax you used to build this assignment? –  q0987 Dec 1 '11 at 16:14
    
@q0987, it's C++ syntax. –  Abyx Dec 1 '11 at 18:08
    
@Loki Astari, I didn't wrote that this code should be placed into one header file. And I didn't wrote anonymous namespace there. –  Abyx Dec 1 '11 at 18:48
    
@Loki Astari, I don't get your first comment. There is no problems with mutex initialization, it will be either initialized with is_destructed initialization, either before it, on GetInstance call. –  Abyx Dec 2 '11 at 5:47
    
@Loki Astari, I updated the code, now it's complete header file (without .cpp file required). –  Abyx Dec 2 '11 at 5:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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