patterncppMinor
Thread-safe Phoenix Singleton class template with Boost
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;
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.
-
-
-
Do you really plan on allowing classes to inherit from
-
This initialization style is mighty strange:
-
When calling a static method or accessing a static variable of a class, inside the given class, prefixing the call with
-
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
-
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.