Thread safe templatized singleton class

Question

#include "stdafx.h"
#include <exception>
#include <string>
#include <sstream>
#include <iostream>
#include <atomic>
template <typename T> 
class Singleton
{
    //thread safe atomic variable to check whether client is creating more than  one instance
    static std::atomic_flag flag ;
public:
    static T& getinstance()
    {
        //I am delibreatly allocating it on heap so that I would not worry about its time of deinitilization 
        //static variable initialization is thread safe
        static T * t = new T();
        return *t;
}
    //exception class- kept it as Internal
    class SingletonException : public std::runtime_error
    {
    public:
    SingletonException(const std::string& exString) :std::runtime_error(exString)
        {

        }
    };
protected:
    Singleton()
    {       
        if (flag.test_and_set())
        {
        std::ostringstream  ex;
            ex << "More than one object of Singleton class " << typeid(T).name();
            throw SingletonException(ex.str());
        }
    }
    ~Singleton()
    {
    }
private:
        Singleton(const Singleton&) = delete;
        Singleton& operator= (const Singleton&) = delete;
        Singleton(Singleton&&) = delete;
        Singleton& operator= (Singleton&&) = delete;
};
template <typename T>
std::atomic_flag  Singleton<T>::flag;
class Myclass: Singleton<Myclass>
{
    public:
        Myclass()
        {
        }
public:
    friend Singleton<Myclass>;
};
int main()
{
try
    {
        Singleton<Myclass>::getinstance();
        Myclass obj;
        Singleton<Myclass>::getinstance();
    }
    catch (const Singleton<Myclass>::SingletonException& ex)
    {
        std::cout << ex.what();
    }
    getchar();
    return 0;
}

Few points:

• I am inheriting it so that I can mark delete copy/move operations, but inheritance is mandatory
• This template class should be kept as friend class of client class
• If more than one object is created then it will throw exception provided client has inherited from it.

It is more like code review. But do you think it is really thread safe GOOD singleton.

Answer 1

Don't use the Singleton pattern

But do you think it is really thread safe good singleton?

Please excuse my pedantry and let me say that the only good use of the Singleton pattern is to not use it at all. This is not to say that a program should never have just a single instance of a class. On the contrary, it is a perfectly reasonable thing at times. Just don't couple the “unique object” property to the type itself as the pattern does. Instead, write a normal class with a public constructor and let another class provide a unique instance of it. The internet is full of rants about the Singleton pattern and good reasons why not to use it so I'll leave it at that.

Watch your #includes

If you're using the typeid operator, you must #include the <typeinfo> header first.

The #include "stdafx.h", on the contrary, is non-portable and annoying. Get rid of it.

For std::runtime_error, you don't need the <exception> but the <stdexcept> header.

Use vertical and horizontal white-space for better readability

It might be just an artifact of copying and pasting your code from your editor into the web browser but as it stands, your code is not very well formatted.

I would put a blank line between functions and classes as well as after the #include directives at the top. The only blank line in your code is – in an empty function body.

There are many opinions about the optimal amount of horizontal space you use to indent your code and I'm okay with most styles. What is not okay is inconsistency. Pick one style and stick to it. Some lines of your code appear completely off. For example, the closing brace of Singleton::getinstance (looks as if it were the end of the class) or the try in main.

Any decent editor can re-indent the code for you at the press of a button.

What's this std::atomic_flag?

I'm not sure what problem exactly the Singleton::flag member is supposed to solve but I'm pretty sure that there's a better way. If you don't want client code to instantiate your singleton class directly (which, as mentioned, is a poor decision, but ignore that for now) just make the constructor private. Then the compiler can enforce at compile-time that nobody except the singleton provider instantiates it. This is much better than checking at run-time and crashing the program.

Reconsider the heap allocation

I don't see why you have to allocate your singleton instance dynamically. The only situation where I could imagine a problem with static deinitialization is if you refer to the object from within a function registered as an std::atexit handler. On the contrary, dynamically allocating the object and then “leaking” it might cause leak detectors such as Valgrind to give you “false” positives that obscure more severe issues with your code. I prefer code to always clean up everything such that it is Valgrind clean. If you're writing a library, it is even more frustrating for your users to get leaks reported that they cannot do anything about.

Returning a reference is fine

I see many Singleton implementations for C++ returning a pointer just because they can. I like your choice of using a reference better.

You don't need an empty destructor

Your inline definition of a non-virtual empty destructor adds nothing. It might only confuse people whether you might have actually forgotten to put a delete of the instance there. (In which case it would of course have been better to use a std::unique_ptr and again write no destructor.)

Make deleted member functions public

It might sound counter-intuitive at first but making deleted members functions public gives you better error messages. It also makes sense if you think about it: The fact that your type is not copyable is part of its public interface.

Did you mean MyClass to inherit privately from Singleton<MyClass>?

I think it would be less surprising if you'd inherit publicly because then you could also say MyClass::getinstance() instead of the more verbose and less intuitive Singleton<MyClass>::getinstance().

The code in MyClass is completely redundant

If you delete everything inside the MyClass body, your program does the exact same thing. I'm sure that you did intend something by writing it but made some mistake (such as accidentally making the constructor public) that renders it useless as it stands.

Get rid of the getchar abomination in main

Don't make your program block on an I/O operation just because you want the terminal emulator window stay open. Instead, learn how to use the emulator such that the window stays open even after the program finished.

If you really want it, it should be std::getchar and you need the <cstdio> header.

How I would do it

Putting the previous advice together, I would solve the problem like this.

template <typename T>
struct singleton_provider
{
  static T&
  instance()
  {
    static T theinstance {};
    return theinstance;
  }
};

struct my_class
{
  my_class(const my_class&) = delete;
  my_class& operator=(const my_class&) = delete;
};

using my_class_provider = singleton_provider<my_class>;

It is a lot simpler, more efficient, doesn't leak memory and – most of all – doesn't couple the logic how objects of a class are instantiated and their lifetime is managed in a program to its type.

Answer 2

Don't use Singleton Pattern

Cut and paste what @5gon12eder said.

Globally accessible mutable state is a bad choice in most situations.

PS: See this brilliant article on how to do it:
https://stackoverflow.com/questions/1008019/c-singleton-design-pattern/1008289#1008289

Don't use the heap:

        //I am delibreatly allocating it on heap so that I would not worry about its time of deinitilization 

So you are going to leak the object. Which means you can't do anything useful in the destructor. So the destuctor is empty so what is there to worry about in deinitilization (or destruction as so many of us call it).

Are you worried about the order of static storage duration object destruction. You know that is not a real issue. There are solutions to that problem. The only reason it is an issue is because people don't know its an issue once you know its an issue the problem is solved because you can enforce the correct order.

See here: Finding C++ static initialization order problems

Creating only one.

I hate the run time check for detecting you have only created one. If you need this you have done it wrong. The whole point is to design it so that it only possible to create one.

protected:
    Singleton()
    {       
        if (flag.test_and_set())
        {
        std::ostringstream  ex;
            ex << "More than one object of Singleton class " << typeid(T).name();
            throw SingletonException(ex.str());
        }
    }

Unique Exception

The reason to have a unique exception is to allow you to catch and fix a specific issue.

    //exception class- kept it as Internal
    class SingletonException : public std::runtime_error
    {
    public:
    SingletonException(const std::string& exString) :std::runtime_error(exString)
        {

        }
    };

Creating more than one singleton is a programming problem that needs to be fixed with testing so it never goes live. Having a unique exception is not useful if you can't do anything with it.