Thread safe Singleton class

[can-mohan]

Guys,

I need to write thread safe singleton class for my assignment. below is the code snippet for factory method in singleton class, I will appreciate if anyone is having suggestion for the same.

*MySingleton * MySingleton::GetInstance()
{


            if (m_pOnlyOneInstance == NULL)
            {
             pthread_mutex_lock(&mutex);
             m_pOnlyOneInstance = new MySingleton();
             pthread_mutex_unlock(&mutex);
            }


      return m_pOnlyOneInstance;
}

[Moschops]

Gargh! Darn double-post edit whackey hackery!

[Moschops]

The atomic action you need to ensure (i.e. the sequence of events that only one thread can enter into) is:

Check for existence of instance.
If it doesn't exist, create it.

Right now, as programmed in your code above, the atomic action is:

If it doesn't exist, create it.

Once you understand what atomic sequence you're trying to create, the fix becomes clear.

[can-mohan]

Thanks for your comment.one thing which I didn't understand that why i need to make a check for existence of instance separately . could you please through some light on this.

[Moschops]

If you don't check for the existence of the singleton, how can your code know if it needs to make one, or just use the one that already exists?

[can-mohan]

But this part is already have taken care in else part .if it is already created then i am returning existing instance and not creating new one.

[Moschops]

But this part is already have taken care in else part .

But it's not thread safe. You need to make it thread safe. What happens if ten separate threads all get to that check at the same time, and all decide that they need to create an object? You get ten objects.

[mike_2000_17]

Moschops is correct, you are "protecting" the wrong part of the code. What could be subject to a race condition is the decision about whether to create the Singleton object or not. The creation of the object itself is not the problem. This would seem to suggest that the correct code would be this:

        pthread_mutex_lock(&mutex);
        if (m_pOnlyOneInstance == NULL)
        {
         m_pOnlyOneInstance = new MySingleton();
        }
        pthread_mutex_unlock(&mutex);

However, there are still causes for concern here, because both the "mutex" object and the m_pOnlyOneInstance pointer need to be initialized some time before this function is called, and if you want to implement the same "contruct-on-first-use" idiom for them, you will have the same race condition problem upon the creation of the mutex object. In fact, any construct-of-first-use global object (Singleton or not) will have this problem. You see how there is no "way out", which is probably why your professor gave this assignment, to teach you about the endless struggle when trying to create thread-safe code.

For the mutex, this is "simple" to solve. In general, any mutex pertaining to a set of threads must be created before the threads are started / created. This avoids all the synchronization issues. So, assuming that the mutex has already been created before starting any thread, then, it should be "safe". The same goes for the initialization of the m_pOnlyOneInstance pointer to NULL. So, to really talk about "thread-safe" implementation, you need to show that the creation of the mutex and the initialization of the m_pOnlyOneInstance pointer is thread-safe (e.g., doing them before any thread is created).

However, this poses an interesting question: If the thread-safety of the creation of the Singleton depends on a couple of things being done before any threads are created, why not just create the Singleton before any threads are created? To me, this question is important from a practical point of view. In real life coding, making something thread-safe just by requiring some other operations to be done before to the threads are created is an oxymoron. Instead, in real life, you would just do everything that is "shared" or "global" before you create threads and then, you don't have to worry about the thread-safety of these operations / initializations.

Well, I could keep on rambling and propose a bunch of convoluted solutions, but there is really only one obvious solution, which is also the primary (recommended) way to implement Singletons:

MySingleton* MySingleton::GetInstance()
{
  static MySingleton UniqueInstance;
  return &UniqueInstance;
}

And yes, this is completely thread-safe! The C++ standard guarantees that the initialization of local static variables is done only once. This includes the case where multiple threads enter the function concurrently. The standard requires that some sort of locking is put in place (under-the-hood) such that the initialization occurs only in one thread, and any other thread will have to wait for the construction to finish before moving on.

I doubt that this will be accepted by your professor (too easy!), especially if he is the one who recommended the kind of singleton creation method that you were using in your original code. But, in real life, this is the solution.

[can-mohan]

Thanks for your comment but it is not thread safe and Their (static variables)order of destruction and construction is not entirely specified in C++, so if you have multiple (different) singletons you can't be entirely sure which one is destroyed first, which is bad if one of the destructors uses the other singleton and there is no locking mechanism to make sure whether it works or not. Below one seems to be right solution as it avoid the race condition between multiple thread and also make sure one instance at a time to maintain atomicity.

MySingleton * MySingleton::GetInstance()
{
            if (m_pOnlyOneInstance == NULL)
        {
        pthread_mutex_lock(&mutex); 

        if(m_pOnlyOneInstance == NULL)
                {

                //m_pOnlyOneInstance = new MySingleton();
        auto_ptr<MySingleton> m_pOnlyOneInstance (new MySingleton());

                }
        pthread_mutex_unlock(&mutex);

         }  




      return m_pOnlyOneInstance;
}*/

[Moschops]

  if you have multiple (different) singletons 

Isn't the point of a singleton that you only have one?

[mike_2000_17]

Thanks for your comment but it is not thread safe and Their (static variables)order of destruction and construction is not entirely specified in C++

You are wrong. It is thread-safe and the order of destruction is deterministic. Here are the relevant parts of the standard:

Here is what the standard says about thread-safety of local static objects (section 6.7/4):

a variable [with static duration] is initialized the first time control passes through its declaration; such a variable is considered initialized upon the completion of its initialization. If the initialization exits by throwing an exception, the initialization is not complete, so it will be tried again the next time control enters the declaration. If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization. (note: The implementation must not introduce any deadlock around execution of the initializer.)

So, the current standard (not the old one) does guarantee that the initialization of local static variables is thread-safe.

And for the order of destruction, that is found in section 3.6.3/1, an exerpt of which is this:

If the completion of the constructor or dynamic initialization of an object with static storage duration is sequenced before that of another, the completion of the destructor of the second is sequenced before the initiation of the destructor of the first. (note: This definition permits concurrent destruction.)

In short, the standard requires obedience to the general principle that "everything is destroyed in the exact opposite order that they were created in". This is even true for static or thread-local objects, whether in global / namespace scope or in a block-scope. The standard is very clear about this.

so if you have multiple (different) singletons you can't be entirely sure which one is destroyed first, which is bad if one of the destructors uses the other singleton and there is no locking mechanism to make sure whether it works or not.

I thought this was just for an assignment? What you are explaining here is just the reason why Singleton's should be avoided as much as possible. This has nothing to do with the order of destruction of anything else, it has to do with bad coding style.

What your original code does is dynamically allocate the singleton object, and thus, the destructors will never be called. You are just leaving the memory to leak (but reclaimed when the program finishes), and leaving the destructor to never be called at all. This isn't exactly "solving" any problem, it's just brushing it under the rug.

First of all, destructors should never do too much. The point of a destructor is to release acquired resources, nothing more. If you have to rely on a singleton within a destructor, it is a sign of poor design.

Second, considering that the main purpose of a destructor is to release resources acquired on construction then this implies that the main reason why you would need to access another singleton in that destructor is to do the release of a resource acquired with that singleton. This implies that this singleton existed when you created the object, and given the guarantees of the C++ standard, this also guarantees that this singleton has not yet been destroyed. This is the whole reason for the "destroy in reverse order" rule.

And, of course, you could come up with twisted situations in which this doesn't work, but the reality is, these are results of flawed designs.

Below one seems to be right solution as it avoid the race condition between multiple thread and also make sure one instance at a time to maintain atomicity.

Really?

First off, you have to show the code that initializes the mutex and the m_pOnlyOneInstance pointer, and how you guarantee that those are executed before any thread attempts to access the singleton. Otherwise, we are just talking in circles.

Second, your code is obviously broken because you have a pointer called m_pOnlyOneInstance that is presumably declared somewhere else and initialized to NULL somewhere before any threads are created. Then, you declare a local variable with the same name, which is an auto_ptr to an object of the Singleton class. The local auto-pointer is not the same pointer as the m_pOnlyOneInstance. This means that Singleton will be created, and then destroyed immediately after (due to the auto-pointer going out of scope), and then the actual m_pOnlyOneInstance will never be set to anything different from NULL.

And by the way, auto_ptr is deprecated, you should use unique_ptr instead.

Even ignoring the smart-pointer use in your code:

MySingleton * MySingleton::GetInstance()
{
  if (m_pOnlyOneInstance == NULL)
  {
    pthread_mutex_lock(&mutex); 
    if(m_pOnlyOneInstance == NULL)
    {
      m_pOnlyOneInstance = new MySingleton();
    }
    pthread_mutex_unlock(&mutex);
  }  
  return m_pOnlyOneInstance;
}

This is still just exactly the same as this:

MySingleton * MySingleton::GetInstance()
{
  if (m_pOnlyOneInstance == NULL)
  {
    pthread_mutex_lock(&mutex); 
    m_pOnlyOneInstance = new MySingleton();
    pthread_mutex_unlock(&mutex);
  }  
  return m_pOnlyOneInstance;
}

Because, unless you marked the m_pOnlyOneInstance pointer to be volatile, the compiler will assume that its value has not changed between the first and second use of the pointer, and thus, will probably not re-evaluate the condition a second time. The use of a mutex does not change this.

Now, you could fix both of these problems by declaring m_pOnlyOneInstance as a volatile std::unique_ptr and default-initialize it before any thread is created. Then, you would have code like this:

MySingleton * MySingleton::GetInstance()
{
  if(!m_pOnlyOneInstance)
  {
    pthread_mutex_lock(&mutex); 
    if(!m_pOnlyOneInstance)
    {
      m_pOnlyOneInstance = std::unique_ptr<MySingleton>(new MySingleton());
    }
    pthread_mutex_unlock(&mutex);
  }  
  return m_pOnlyOneInstance.get();
}

And the singleton will be destroyed when the std::unique_ptr goes out of scope (which, again, is deterministically determined by when you first default-initialized it).

And now, we are back to the initial problem I explained in the previous post: all the "thread-safety" guarantees depend on when and how you create and initialize the mutex and the pointer.

Isn't the point of a singleton that you only have one?

I assume he meant that he has different singletons (of different classes) within the same program. In reality, he is really just talking about global variables (singletons being just one kind of global variable), because the fact that the class is a Singleton or not is irrelevant to the whole thread-safety of the "construct-on-first-use" pattern, which is really the central issue here.

[can-mohan]

This is the same one i had shared(Also known meyer's singleton) earlier during my assignment but it was rejected due to compiler dependency as this solution is not portable and works on afew compiler. You are right i need to use unique_ptr instead of auto_pointer as it is not used now days due to its copying behaviour. I also need to make sure about minimum synchronization and locking efforts in my singleton pattern. Current shared pattern is based on Double check locking . May be we can minimize such calls by caching the pointer that instance returns. For example

MySingleton::instance()->method1();
Singleton::instance()->method2();
Singleton::instance()->method3();
Locking minimal implementation:
Singleton*const instance =
Singleton::instance();
// cache instance pointer
instance->method1();
instance->method2();
instance->method3();

[mike_2000_17]

Current shared pattern is based on Double check locking . May be we can minimize such calls by caching the pointer that instance returns. For example

The locking will occur only the first time you call the instance() function, and possibly locking some concurrent calls. But after that, there won't be any locking, just a check. So, caching the pointer is really only to limit the redundant checks, or, more importantly, the memory fence.

I'm afraid some people might be a bit lost with all this partial code, so, just to put things straight, here is the current solution you are talking about:

// in the header file:

#include <mutex>
#include <memory>
#include <atomic>

class Singleton {
  public:
    static Singleton* instance();

    void method1();
    void method2();
    void method3();
    //...

  private:
    Singleton() { };

    // thread-sync vars:
    static std::unique_ptr<Singleton> p_inst;
    static std::atomic_bool is_created;
    static std::mutex creation_mutex;
};

// in the .cpp file:

// statically initialized global variables:
std::unique_ptr<Singleton> Singleton::p_inst = std::unique_ptr<Singleton>();
std::atomic_bool Singleton::is_created = std::atomic_bool(false);
std::mutex Singleton::creation_mutex = std::mutex();

// lazy-initialized Singleton object:
Singleton* Singleton::instance() {
  if( !is_created.load() ) {
    std::unique_lock lock_here(creation_mutex);
    if( !is_created.exchange(true) ) {
      p_inst = std::unique_ptr<Singleton>(new Singleton());
    };
  };
  return p_inst.get();
};

As far as I know, this is the only completely thread-safe way to perform the initialization of the Singleton object in the way that you described (double-checked lock, notice what the C++11 section says..).

Of course, the easiest and most robust way to do this in C++ is:

// in the header file:

class Singleton {
  public:
    static Singleton* instance();

    void method1();
    void method2();
    void method3();
    //...

  private:
    Singleton() { };
};

// in the .cpp file:

// lazy-initialized Singleton object:
Singleton* Singleton::instance() {
  static Singleton inst;
  return &inst;
};

Which comes with all the same thread-safety guarantees, from any reasonably standard-compliant compiler.

The only difference between those two is the order of destruction. In the first case (with the global unique-pointer), the destruction of the Singleton object is going to happen at some unknown point in the midst of the destruction of other global variables (statically initialized). In the second case, the destruction of the Singleton is going to happen before any global variables are destroyed and in the exact reverse order that all static-duration dynamically-initialized objects were constructed. I tend to feel safer with the latter.

As far as the double-checked locking goes, this is most likely the way that the compiler implements the locking on all static-duration dynamically-initialized objects. Because the standard requires that behavior, and implicitely requires that implementation (or some platform-specific optimized version of it).

This is the same one i had shared(Also known meyer's singleton) earlier during my assignment but it was rejected due to compiler dependency as this solution is not portable and works on afew compiler.

If by "not portable" you mean that it requires a standard-compliant compiler, then I guess that's true. As far as I know, the only compiler that does not yet make static object initialization thread-safe is the Microsoft compiler (MSVC). GCC has supported it since version 4.3 (2008) and Clang has supported it since version 2.9. I'm not sure about ICC, but they are pretty huge in the field of concurrency, and they recommend the use of static local variables instead of the double-checked locking variants.

So, decent compilers (GCC, ICC, Clang) have supported this for years. And if we are going to start considering everything that MSVC does not support or implements incorrectly, then we're in for a long ride, because that list is long.

I guess my point is that portability is relative, it's a matter of how far back you go and what wacky compilers you include in your list.

If what you want is a C++03 implementation of a thread-safe Singleton creation, then this is what you are looking for:

// in the header file:

#include <boost/atomic.hpp>
#include <boost/thread/mutex.hpp>

class Singleton {
  public:
    static Singleton* instance();

    void method1();
    void method2();
    void method3();
    //...

  private:
    Singleton() { };
    static Singleton* instance_impl();

    // thread-sync vars:
    static boost::atomic<bool> is_created;
    static boost::mutex creation_mutex;
};

// in the .cpp file:

// statically initialized global variables:
boost::atomic<bool> Singleton::is_created = boost::atomic<bool>(false);
boost::mutex Singleton::creation_mutex = boost::mutex();


// lazy-initialized Singleton object:
Singleton* Singleton::instance_impl() {
  static Singleton inst;
  return &inst;
};

// lazy-initialized Singleton object:
Singleton* Singleton::instance() {
  if( !is_created.load() ) {
    boost::mutex::scoped_lock lock_here(creation_mutex);
    if( !is_created.exchange(true) ) {
      return instance_impl();
    };
  };
  return instance_impl();
};

[can-mohan]

Good one,It seems to be perfect . I really appreciate your efforts.

[mike_2000_17]

I just realized that my solutions are not exception-safe. The problem is that I set the is_created bool to true before making sure the constructor has completed. This is the way the core part should be done:

if( !is_created.load() ) {
  p_inst = std::unique_ptr<Singleton>(new Singleton());
  is_created.store(true);
};

or, for the other version:

if( !is_created.load() ) {
  Singleton* result = instance_impl();
  is_created.store(true);
  return result;
};