HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Thread-safe Phoenix Singleton class template with Boost

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
templatewithphoenixsingletonthreadsafeclassboost

Problem

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
#include
#include

template
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 _instance;
static boost::atomic _destroyed;
static boost::mutex _instantiation_mutex;
};

template
boost::atomic TSingleton::_destroyed = (boost::atomic) false;

template
boost::atomic TSingleton::_instance = (boost::atomic) nullptr;

template
boost::mutex TSingleton::_instantiation_mutex;

template
TSingleton::~TSingleton(){
TSingleton::_destroyed = true;
TSingleton::_instance = nullptr;
}

template
T& TSingleton::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
void TSingleton::_CreateInstance(){
static T instance;
TSingleton::_instance.store(&instance, boost::memory_order_release);
}

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

Solution

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) 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.

Context

StackExchange Code Review Q#70366, answer score: 4

Revisions (0)

No revisions yet.