Modern C++ Singleton Template

Question

I recently read about the C++17 static inline member declaration and thought that this will make templates a little bit cleaner, since static members can now be initialized inside a templated class.

Because of this I wanted to create a neat little Singleton template (since it is the perfect example where static members are needed).

Now to my questions: Is there something I potentially missed, i.e. are there possibilities to create copies of a derived Singleton? Is it a good idea to use CRTP for a Singleton in general? What a about the move constructor, do I need to handle it as well?

Here is the template:

template < typename T >
class Singleton {
  public:
    static T& GetInstance() {
      static MemGuard g; // clean up on program end
      if (!m_instance) {
        m_instance = new T(); 
      }
      return *m_instance;
    }

    Singleton(const Singleton&) = delete;
    Singleton& operator= (const Singleton) = delete;

  protected:
    Singleton() { };
    virtual ~Singleton() { }

  private:
    inline static T * m_instance = nullptr;

    class MemGuard {
      public: 
        ~MemGuard() {
          delete m_instance;
          m_instance = nullptr;
        }
    };
};

And here a possible derived type:

class Test final : public Singleton<Test> {
  friend class Singleton<Test>;
  public:
    void TestIt() { };
  private:
    Test() {}  
    ~Test() { /* Test intern clean up */ }
};

Answer 1

Okay so first of the obligatory Singletons are bad practice so you probably shouldn't make it easy to write bad code.

Ignoring the fact that the class probably shouldn't exist at all we can look at the code.

static T& GetInstance() {
  static MemGuard g; // clean up on program end
  if (!m_instance) {
    m_instance = new T(); 
  }
  return *m_instance;
}

If multiple threads access this instance simultaneously before it is created, you have a data race and m_instance may end up be being constructed multiple times or other kinds of undefined behaviour. You need to add mutex locks around the if block or use std::call_once which is preferred.

As it is supposed to be a singleton you're not supposed to be able to create more instances as the meaning of a singleton is to just have one instance but it appears that it is fully possible to construct multiple instances of Test simply by creating them as local variables. So this is a design flaw in your template.

A much better way of creating a singleton is to rely on C++11 Magic Statics (N2660). And simply do this:

class Test{
private:
    Test(); // Disallow instantiation outside of the class.
public:
    Test(const Test&) = delete;
    Test& operator=(const Test &) = delete;
    Test(Test &&) = delete;
    Test & operator=(Test &&) = delete;

    static auto& instance(){
        static Test test;
        return test;
    }
}; 

Which is much easier to write than your code, it's thread safe and fixes the issues with allowing Test to be instantiated. The properties of magic statics guarantee that test will be initialised exactly once the first time the function body is entered by any thread, even in the presence of multiple threads that might otherwise cause a data-race. The instance will be deconstructed when your main() function returns (in the static destruction stage) which makes the whole MemGuard thing unnecessary.

Answer 2

Singletons make it hard to test your code, and in my job I'd reject this at review for encouraging the development of untestable features. That said, I'll continue reviewing despite that.

No need for helper class

The MemGuard appears to be a poor man's reimplementation of std::unique_ptr. It would be much simpler for you to declare m_instance as a std::unique_ptr<T>, and then just return *m_instance from your accessor.

There's a race condition when two or more threads try to create the instance (when both see a null pointer "before" the other has set it). You could work around this with a mutex lock, but it's simpler to use a local static variable, which is thread-safe:

#include <memory>
template<typename T>
T& Singleton<T>::instance()
{
    static const std::unique_ptr<T> instance{new T{}};
    return *instance;
}

---

We don't need a destructor

There's no need for the empty virtual destructor, as the constructed object will always be deleted as its declared type.

---

Revised implementation

With my changes, the code reduces to

template<typename T>
class Singleton {
public:
    static T& instance();

    Singleton(const Singleton&) = delete;
    Singleton& operator= (const Singleton) = delete;

protected:
    struct token {};
    Singleton() {}
};

#include <memory>
template<typename T>
T& Singleton<T>::instance()
{
    static const std::unique_ptr<T> instance{new T{token{}}};
    return *instance;
}

I'm using a constructor token to allow the base class to call the subclass's constructor without needing to be a friend.

Example

An example T looks like:

#include <iostream>
class Test final : public Singleton<Test>
{
public:
    Test(token) { std::cout << "constructed" << std::endl; }
    ~Test() {  std::cout << "destructed" << std::endl; }

    void use() const { std::cout << "in use" << std::endl; };
};

Although the constructor is public, it can't be called without a Singleton<T>::token object, meaning that access to it is now controlled.

Tests:

int main()
{
    // Test cannot_create; /* ERROR */

    std::cout << "Entering main()" << std::endl;
    {
        auto const& t = Test::instance();
        t.use();
    }
    {
        auto const& t = Test::instance();
        t.use();
    }
    std::cout << "Leaving main()" << std::endl;
}

Entering main()
constructed
in use
in use
Leaving main()
destructed

---

Afterthought:

There's no need for the smart pointer; ordinary memory management works here:

template<typename T>
T& Singleton<T>::instance()
{
    static T instance{token{}};
    return instance;
}

Answer 3

When working with static and shared libraries, one must be careful that you don't have several implementations of the instance() function. That would lead to hard to debug errors where there actually would exist more than one instance. To avoid this use an instance function inside a compilation unit (.cpp) and not in a template from a header file.