Generic Singleton (Thread Safe)

Question

LifeTracker.h

#ifndef _LIFETRACKER_H_
#define _LIFETRACKER_H_
#include <vector>
#include <algorithm>
#include "MySIngleton.h"
class LifetimeTracker
{
public:
 LifetimeTracker(unsigned int x) : longevity_(x) {}
 virtual ~LifetimeTracker() = 0;
 friend bool Compare(const LifetimeTracker* p, const LifetimeTracker* q);
 void setKey(const std::string& key)
 {
 key_ = key;
 }
 const std::string& getKey()
 {
 return key_;
 }
 void setLongevity(unsigned int nlongevity)
 {
 longevity_ = nlongevity;
 }
private:
 //This is the relative age of Singleton object(s) 
 unsigned int longevity_;
 //Stored this Key to Identify Singleton Objects, if client want to Set Longivity
 std::string key_;
};
inline bool Compare(const LifetimeTracker* p, const LifetimeTracker* q)
{
 return p->longevity_ < q->longevity_;
}
// Definition required
LifetimeTracker::~LifetimeTracker() = default;
//Helper destroyer function
// Concrete lifetime tracker for objects of type T
template <typename T, typename Destroyer>
class ConcreteLifetimeTracker : public LifetimeTracker
{
public:
 ConcreteLifetimeTracker(T* p, unsigned int longevity, Destroyer d) : LifetimeTracker(longevity)
 , pTracked_(p)
 , destroyer_(d)
 {
 }
 ~ConcreteLifetimeTracker()
 {
 destroyer_(pTracked_);
 }
private:
 //This is the tracked object
 T* pTracked_;
 //This is destroying method 
 Destroyer destroyer_;
};
//This will be given to std::atexit
void AtExitFn(); // Declaration needed below
template <typename T>
struct Deleter;
template <typename T, typename D >
class Singleton;
//This is generic deleter, but client can also give its deleter
template <typename T>
struct Deleter
{
 void operator()(T* pObj)
 {
 Singleton<T, Deleter<T> >::Destroy(pObj);
 }
};
template <typename T, typename D>
std::string getObjectKey()
{
 std::string key = typeid(T).name();
 key = key + typeid(D).name();
 return key;
}
typedef std::vector<LifetimeTracker*> TrackerArray;
TrackerArray pTrackerArray;
unsigned int Priority = 0;
template <typename T, typename Destroyer >
int SetLongevity(T* pobj, Destroyer d, unsigned int nlogevity = 0)
{
 if (nlogevity == 0)
 {
 nlogevity = ++Priority;
 }
 LifetimeTracker* ptracker = new ConcreteLifetimeTracker<T, Destroyer>(pobj, nlogevity, d);
 ptracker->setKey(getObjectKey<T, Destroyer>());
 pTrackerArray.push_back(ptracker);
 return 1;
}
template <typename T, typename Destroyer = Deleter<T> >
void SetLongevity(unsigned int nlogevity)
{
 for (auto i : pTrackerArray)
 {
 if (i->getKey() == getObjectKey<T, Destroyer>())
 {
 i->setLongevity(nlogevity);
 }
 }
}
void AtExitFn()
{
 sort(pTrackerArray.begin(), pTrackerArray.end(), Compare);
 for (auto i : pTrackerArray)
 {
 delete *&i;
 }
 pTrackerArray.erase(pTrackerArray.begin(), pTrackerArray.end());
}
#endif // !_LIFETRACER_H

MySIngleton.h

#ifndef _MYSINGLETON_H_
#define _MYSINGLETON_H_
#include "LifeTracker.h"
#include <string>
//If client Inherit from this class, it needs not to private or delete it all construc operations
class NoConstructOperation
{
protected:
 NoConstructOperation() = default;
 virtual ~NoConstructOperation() = default;
public:
 NoConstructOperation(const NoConstructOperation&) = delete;
 NoConstructOperation& operator =(NoConstructOperation&) = delete;
 NoConstructOperation(NoConstructOperation&&) = delete;
 NoConstructOperation& operator = (NoConstructOperation&&) = delete;
};
// If client call Singleton<Myclass>, Singleton<Myclass, Deleter> it will give two different instance of Myclass
template
<
 typename T, typename D = Deleter<T>
>
class Singleton : public NoConstructOperation
{
public:
 //Check availabily of construcor, copy/move construction/assignment
 //{
 static_assert(!std::is_constructible<T>::value, "Constructor is public");
 static_assert(!std::is_trivially_constructible<T>::value, "Compiler provided default constructor is public: Please declare it private");
 static_assert(!std::is_trivially_copyable<T>::value, "Compiler provided copy constructor is avaliable");
 static_assert(!std::is_copy_constructible<T>::value, "copy constructor is avaliable");
 static_assert(!std::is_trivially_copy_assignable<T>::value, "Compiler provided assignment is avaliable");
 static_assert(!std::is_copy_assignable<T>::value, "copy assisgnment is avaliable");
 static_assert(!std::is_trivially_move_assignable<T>::value, "Compiler provided move assisgnment is avaliable");
 static_assert(!std::is_move_assignable<T>::value, "move assisgnment is avaliable");
 static_assert(!std::is_move_constructible<T>::value, "move constructor is avaliable");
 static_assert(!std::is_trivially_move_constructible<T>::value, "Compiler provided move constructor is avaliable");
 //}
 static T& Instance();
 static void Destroy(T* obj);
private:
 static T* CreateInstance();
 static int ScheduleForDestruction(void(*)());
};
template<typename T, typename D>
T& Singleton<T, D>::Instance()
{
 //This will be thread safe ?
 static T* ptr = CreateInstance();
 // I have overloked it, this is for thread safety
 static int i = SetLongevity<T, D>(ptr, D());
 static int i = ScheduleForDestruction(&AtExitFn);
 return *ptr;
}
template<typename T, typename D>
inline T* Singleton<T, D>::CreateInstance()
{
 return new T();
}
template<typename T, typename D>
inline int Singleton<T, D>::ScheduleForDestruction(void(*pFun)())
{
 std::atexit(pFun);
 return 1;
}
template<typename T, typename D>
inline void Singleton<T, D>::Destroy(T* obj)
{
 delete obj;
 obj = nullptr;
}
#endif

Client Code

// C++11/14 thread safe singleton
#include "stdafx.h"
#include <typeinfo>
#include <stdexcept>
#include <stdlib.h>
#include <exception>
#include <string>
#include <sstream>
#include <iostream>
#include <atomic>
#include <cstdio>
#include "MySIngleton.h"
class Deleter1;
class Myclass : public NoConstructOperation
{
public:
 friend class Deleter1;
 friend class Singleton<Myclass>;
 friend class Singleton<Myclass, Deleter1>;
private:
 Myclass() = default;
 ~Myclass() = default;
};
class Deleter1
{
public:
 void operator()(Myclass *p)
 {
 delete p;
 p = nullptr;
 }
};
class Myclass1 :public NoConstructOperation
{
public:
 friend Singleton<Myclass1>;
};
int main()
{
 Singleton<Myclass, Deleter1>::Instance();
 SetLongevity<Myclass, Deleter1>(6);
 Singleton<Myclass>::Instance();
 getchar();
 return 0;
}

This is Thread safe singleton. LifeTracker - will track the lifespan of Singleton Objects through longivity variable and through std::atexit, we will clean Singletons.

Singleton - Which has class of which instance will be created + a destroyer which if client does not provide , Single will clean by itself.

Pleae ask more details if you require to understand it.

• Loki Astari: What advantage does this design have over the standard Singelton implementation? stackoverflow.com/a/1008289/14065
• gaurav bharadwaj: Although, I know above implementation of code has some bug which I found in reviewing. As for your question about the advantages of this implementation over the Singelton you mentioned so they are briefly: 1) This singelton is templatized. 2)It enforces that client class should not have `public construct/move operations. 3)It addresses the problem of order of destruction of singleton objects. 4)Even client can decide the relative age of their Singleton objects. (although I have to make that thread safe).
• orip: Unrelated to the usefulness of the code, it seems you're defining symbols outside a namespace, you're including headers you don't use, and try to make the #ifndef/#define symbol unique - e.g by adding a long random suffix to it - to prevent collisions.
• Loki Astari: 1) Templatization. Not a big deal. Youcan templatize the standard one. 2) Sure. 3) Not really (in fact using atexit() makes the problem harder). The order of destruction of a singelton is not really that big a deal. Solving SIOP 4) Not sure what that means. But the standard pattern allows for lazy initialization as well.
• gaurav bharadwaj: 4) point in my comment was related to define the life span of objects so if there are two class A and B and client want to A live longer than B. He can call setLongivity function passing greater interger for A. Anyhow I am wondering @LokiAstari why atexit makes problem harder

Answer 1

Singelton Anti-Pattern (@Jerry ;-)

OK. I assume that you know Singelton is also considered an anti-pattern. If you think about it there are usually better ways of doing things.

Design

You have implemented some code that guarantees an order of destruction on your singletons (hopefully there are not that many that this is a big worry!). It looks like it should work.

Couple of issues I have are:

• Specifying the order of destruction is manual and you have to call SetLongevity() for the type (and destructor). This is very fragile especially in long lived code where things are changed without knowing the context of the whole program.
• It uses atexit().
  The functions registered with atexit() are run before the destruction of static storage duration objects. This means that your singletons that are accessed from the destructor of these objects will always be accessing destroyed objects.

The other issue I have with this is that current standard technique already handles the order of creation and destruction correctly.

See: Finding C++ static initialization order problems

Once you understand there is an order of initialization/destruction problem it goes away because the solution is so simple. The only problem with order of initialization problem is that people are not aware of it.

Code Review

Underscore and Identifier

The rules of using underscore in an identifier are non trivial and most people get it wrong. So best to avoid prefixing identifiers with underscore.

For full details read this: What are the rules about using an underscore in a C++ identifier?

#ifndef _LIFETRACKER_H_
#define _LIFETRACKER_H_

The above is illegal. These identifiers are reserved for the implementation. Using them results in an invalid program.

Put your local header file first.

#include <vector>
#include <algorithm>
#include "MySIngleton.h" <--- This should be first.

You are trying to prevent your header file being accidently dependant on other people putting header file first. Your header file should contain all the other includes that it needs but no more. By putting it third in the list you may potentially hide the error of a missing dependency; eg. that your file depends on and you forgot to put #include <vector> in your header file. When somebody else uses your header file they find they need to add a #include <vector> to their code even if they are not using it because you forgot it.

Namespace

Put your code in a namespace. Some of your functions have very common names that could cause accidental errors in other people's code' Compare and Deleter springs to mind.

Unsigned

The only time that is real use case for unsigned int is for a bit field (and because the committee made size() return unsigned by mistake (they regret that decision)). Otherwise the automatic compiler conversion from signed to unsigned can cause some unexpected problems.

LifetimeTracker life(-15); // Compiles just fine.

The problem here is that user would expect that this object would be destroyed first (because it has a negative number and all others have a positive). But because the compiler will silently convert this value to a an unsigned value with a very big positive value you will get the exact opposite behavior of what your user expects.

Principle of least surprise has been violated here.

Avoid Get/Set functions

This breaks encapsulation. You are exposing implementation details of your class to the world. Methods should be actions (verbs) that manipulate the state of the object.

• Do you really want to reset the Key after it has been created?
  • Why not just set the key when it is created?
• Why are you getting the key?
  • Is it just to do a comparison?
  • Why not write an appropriate comparison operator?

All will make the use of the class more logical!

OK. setLongevity() is a valid use case. The user has requested the longevity been updated. Ahh now we see a better name updateLongevity() it already had a value and the user has requested you update it!

class LifetimeTracker
{
 void setKey(const std::string& key) 
 const std::string& getKey()
 void setLongevity(unsigned int nlongevity);
};

Const methods

Methods that don't mutate the state of the object should be marked const.

 const std::string& getKey() -> const <- You forgot a const.
 {
 return key_;
 }

Unused return value

If your function always returns the same value. Then you have no need of a return value. Just make it void

template <typename T, typename Destroyer >
int SetLongevity(T* pobj, Destroyer d, unsigned int nlogevity = 0)
{
 // STUFF
 return 1; // Who cares that it returns 1.
}

Smelly

This smells.

 delete *&i;

Why are you doing all the *& taking the address and then de-referencing it?

Clear and erase all

 pTrackerArray.erase(pTrackerArray.begin(), pTrackerArray.end());

Sure that works. But it is the same as calling clear. And the method clear() much more accurately describes intent.

Thread Safety

Yes this is thread safe. Since C++11 the standard has guaranteed that this object will only be initialized once (even in the presence of threads).

 //This will be thread safe ?
 static T* ptr = CreateInstance();

Even before C++11 several compilers made that gurantee (but not all of them).

This is thread safe in that it will be only initialized once.

 // I have overloked it, this is for thread safety
 static int i = SetLongevity<T, D>(ptr, D());

It is not thread safe in the context that it may be initialized by a different thread than ptr above. Your only guarantee is that it is initialized once.

Same issue here. If it is a problem.

 static int i = ScheduleForDestruction(&AtExitFn);

Seems like a lot of work

template<typename T, typename D>
inline T* Singleton<T, D>::CreateInstance()
{
 return new T();
}

This is only used in one place. Why not just put new T{} at the place where this function is called?

• gaurav bharadwaj: CodeReview guideline discourage to say thanks but I couldn't stop myself . Thanks @Loki, I incorporated your suggestions in above code.
• KeyC0de: Martin Maybe I'm missing something. Isn't a Meyer's Singleton (static function instance) thread safe by default?
• Loki Astari: @Nik-Lz: Post C++11 yes. Pre C++11 technically no (but gcc had an explicit fix to make it thread safe but this was not required by the standard (because before C++11 there was no concept of threads in the standard and thus no guarantees about it)).