6
\$\begingroup\$

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;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! Your question looks like an interesting implementation and I hope you get good reviews! \$\endgroup\$ – Phrancis Nov 20 '14 at 2:42
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ - _destroyed = false; does not work on VS13 (see Initializing std::atomic_bool? ). I use Boost in my project anyway - but I could rewrite it for plain C++11 if rquested (but thats offtopic :). \$\endgroup\$ – PatrickP Nov 20 '14 at 17:02
  • \$\begingroup\$ @PatrickP - That's quite surprising. Well, according to the answers, you can still use the constructor initialization, which would be the most correct form anyway. \$\endgroup\$ – glampert Nov 20 '14 at 17:05
  • \$\begingroup\$ Already done. I will change the code according to your suggestions and update the question. \$\endgroup\$ – PatrickP Nov 20 '14 at 17:07
  • 2
    \$\begingroup\$ @PatrickP - You mentioned updating the question. Read this first: meta.codereview.stackexchange.com/questions/1763/… \$\endgroup\$ – glampert Nov 20 '14 at 17:12
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I can understand that you disrelish the Phoenix Singleton. But this is imho the lesser of the evils-solution for my problem. I need the singleton for managing global states seamlessly from c++ and an embedded python environment throughout different threads. as if this wasn't enough, in python you can create derivate from the singleton or use the singleton indirect from another c++ static through a python call - so I have no control about the lifetime with the accessing it in the constructor method you've mentioned. \$\endgroup\$ – PatrickP Nov 21 '14 at 2:16
  • \$\begingroup\$ TSingleton<T>::~TSingleton(){...} I agree - design flaw. Shared lib problem: I agree in general case, but the Singleton is in my context only used in main-binary and is not used in extern code. \$\endgroup\$ – PatrickP Nov 21 '14 at 2:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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