Thread-safe Phoenix Singleton class template with Boost

Question

I've implemented the phoenix-singleton as a class template with boost inspired by Modern C++ Design. The code compiles fine with MSVC 2013 - and it seems to work, but I'm new to multi-threaded programming and Boost. Are there any pitfalls in the code? Improvements?

TSingleton.h

#ifndef TSINGLETON_H
#define TSINGLETON_H

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

template<typename T>
class TSingleton{
public:
    static T& getInstance();

protected:
    inline explicit TSingleton();
    TSingleton(const TSingleton&);
    TSingleton& operator=(const TSingleton&);
    virtual ~TSingleton();
    static void _CreateInstance();
    static void _Phoenix();
    static void _KillPhoenix();

    static boost::atomic<T*> _instance;
    static boost::atomic<bool> _destroyed;
    static boost::mutex _instantiation_mutex;
};



template<typename T>
boost::atomic<bool> TSingleton<T>::_destroyed = (boost::atomic<bool>) false;

template<typename T>
boost::atomic<T*> TSingleton<T>::_instance = (boost::atomic<T*>) nullptr;

template<typename T>
boost::mutex TSingleton<T>::_instantiation_mutex;

template<typename T>
TSingleton<T>::~TSingleton(){
    TSingleton::_destroyed = true;
    TSingleton::_instance = nullptr;
}

template<typename T>
T& TSingleton<T>::getInstance(){
    T* tmp = TSingleton::_instance.load(boost::memory_order_consume);
    if (!tmp){
        boost::mutex::scoped_lock guard(TSingleton::_instantiation_mutex);
        tmp = TSingleton::_instance.load(boost::memory_order_consume);
        if (!tmp){
            if (TSingleton::_destroyed){
                TSingleton::_Phoenix();
            }else{
                TSingleton::_CreateInstance();
            }
            tmp = TSingleton::_instance.load(boost::memory_order_consume);
        }
    }
    return *tmp;
}

template<typename T>
void TSingleton<T>::_CreateInstance(){
    static T instance;
    TSingleton::_instance.store(&instance, boost::memory_order_release);
}

template<typename T>
void TSingleton<T>::_Phoenix(){
    TSingleton::_CreateInstance();
    new(TSingleton::_instance) T;
    std::atexit(TSingleton::_KillPhoenix);
    TSingleton::_destroyed = false;
}

template<typename T>
void TSingleton<T>::_KillPhoenix(){
    T* tmp = TSingleton::_instance.load(boost::memory_order_consume);
    tmp->~T();
    TSingleton::_destroyed = true;
    TSingleton::_instance = nullptr;
}

#endif // TSINGLETON_H

Simple usage example:

#include <iostream>
#include "TSingleton.h"

class Foo{
public:
    void bar(){ std::cout << "bar" << std::endl; }
};

int main(int argc, char **argv){
    Foo& myfoo = TSingleton<Foo>::getInstance();
    myfoo.bar();
    return 0;
}

Answer 1

I'm not a threading specialist, so won't be able to tell you for sure if your code is 100% race condition free. However, there are a few other code smells that I would like to warn you about:

• Reserved names: A name starting with an underscore and an uppercase letter is reserved in any scope. _CreateInstance, _Phoenix and _KillPhoenix are not safe to be used, even inside a class.

• inline is redundant for a template class or function. The class is always inline, so you don't have to add the keyword.

• explicit makes no sense for a constructor that takes 0 arguments. You usually apply explicit if you have a constructor taking 1 argument and you wish to avoid an implicit conversion.

• Do you really plan on allowing classes to inherit from TSingleton? It doesn't seem like there is a need for that, since the singleton type is a template parameter. Maybe I'm missing something, but if it is not to be inherited, make all currently protected members private and remove virtual from the destructor.

• This initialization style is mighty strange: _destroyed = (boost::atomic<bool>) false; A plain assignment _destroyed = false; or using the constructor syntax: _destroyed(false); would be much better.

• When calling a static method or accessing a static variable of a class, inside the given class, prefixing the call with ClassName is optional. So you could, for example, simply use _instance instead of TSingleton::_instance.

• VS 2013 should have most if not all C++11 features and libraries working. So you could replace Boost with the new threading and atomics libraries to reduce dependencies.

• If you switch to C++11, you can delete the copy constructor and assignment operator.

Answer 2

Phenix Singelton: I hate this whole idea.

There are better ways of doing it.

The classic singleton solves most of your problems.

class S
{
    public:
        static S& getInstance()
        {
            static S    instance; // Guaranteed to be destroyed.
                                  // Instantiated on first use.
            return instance;
        }
    private:
        S() {};
        S(S const&);              // Don't Implement
        void operator=(S const&); // Don't implement
};

So the only thing you need to worry about is when the singleton is destroyed and you want to re-use it. Since the classic singelton is a static storage duration object it will be destroyed after main() exits. Thus the only time you need to worry about accessing a destroyed signelton is if you access it from the destructor of another static storage duration object (global variable).

This is not really a problem as you can force the singelton to live longer than your object by accessing it in the constructor. So simple rule. If you access a classic singelton from the destructor just make sure you access it in the constructor first (and then all your problems are solved).

Lot of description here: Finding C++ static initialization order problems

Code Review

destructor?

You really want to set things to NULL when an object is destroyed!!!

template<typename T>
TSingleton<T>::~TSingleton(){
    TSingleton::_destroyed = true;
    TSingleton::_instance = nullptr;
}

This will cause this to Fail:

Foo& getMyFoo()
{
    TSingleton<Foo>   x;
    return x.getInstance();
}   // x goes out of scope here and resets the class members to null via detructor.
    // Yep I know this is not the use case you designed it for.
    // But this is still technically allowed.

void doWork()
{
   Foo&   x = getMyFoo();  // Reference to object.  // But the pointer that originally
                                                    // held it is now NULL (x is good)

   Foo&   y = getMyFoo();                           // Here you are forcing a 
                                                    // re-construction of your object but
                                                    // without the destructor being called.
                                                    // Which I am pretty sure is UB.
}

Shared lib problem.

This is not going to interact well with shared libraries on all systems.

I seem to remember (though I could be wrong here). That using this template technique is not going to react well to being used across multiple shared libraries. Because it is a template it is not going to be removed if you have multiple versions of this singelton in different shared libraries.

@glampert has mentioned all the other points I want to mention.