Unit-testing friendly singleton

Question

In my project we have a few singletons, which tends to be problematic in unit tests. So I wanted to find solution for the problem. Here is what I came with so far:

smart_singleton.h

class smart_singleton
{
public:
    static std::shared_ptr<smart_singleton> get_instance();

private:
    smart_singleton();

    smart_singleton(const smart_singleton&) = delete;
    smart_singleton operator=(const smart_singleton&) = delete;
    smart_singleton(smart_singleton&&) = default;
    smart_singleton& operator=(smart_singleton&&) = default;

    static std::weak_ptr<smart_singleton> weak_instance;
};

smart_singleton.cpp

std::weak_ptr<smart_singleton> smart_singleton::weak_instance;

std::shared_ptr<smart_singleton> smart_singleton::get_instance()
{
    if (auto existing_instance = weak_instance.lock()) {
        return existing_instance;
    } else {
        std::shared_ptr<smart_singleton> tmp_shared(new smart_singleton());
        weak_instance = tmp_shared;
        return tmp_shared;
    }
}

smart_singleton::smart_singleton()
{

}

The difference is that you need to hold one shared_ptr from "get_instance()" anywhere in your code for the object to not be destroyed. In your prodution code that would be somewhere in the main function (or some object that is alive for the whole scope of main). In UT that would be for the duration of one test.

IMPORTANT:
Please don't focus on the singleton pattern itself being discouraged. My project has singletons and changing that would mean a lot of effort. My attempt is only to make unit testing easier. What I'm interested in, therefore, are the differences between having a classic implementation of singleton in C++ and my implementation and what possible problems could the latter one cause.

Answer 1

I like the idea.

First off, it appears that the get_instance() method isn't thread-safe:

std::shared_ptr<smart_singleton> smart_singleton::get_instance()
{
    if (auto existing_instance = weak_instance.lock()) {
        return existing_instance;
    } else {
        std::shared_ptr<smart_singleton> tmp_shared(new smart_singleton());
        weak_instance = tmp_shared;
        return tmp_shared;
    }
}

It's fine for it to not be thread-safe, but in that case, it is very important for the documentation to make it clear. If you are going that route, however, it might be worth separating the get_instance from creation like so:

std::shared_ptr<smart_singleton> smart_singleton::create_instance()
{
    if (weak_instance.lock()) {
        throw already_instantiated_error{}; // something better
    }
    std::shared_ptr<smart_singleton> tmp_shared(new smart_singleton());
    weak_instance = tmp_shared;
    return tmp_shared;
}

std::shared_ptr<smart_singleton> smart_singleton::get_instance()
{
    return weak_instance.lock();
}

Then you can note in the documentation that get_instance() returns a nullptr if there is no instance present; in a "global" scope (like main), they should call auto keep_alive = smart_singleton::create_instance();.

---

smart_singleton.h needs header-guards, and it should include <memory>:

#ifndef SMART_SINGLETON_H_
#define SMART_SINGLETON_H_

#include <memory>

...

#endif

---

To me, the main problem with this design is that it is so easy to use wrong. Any class caching a static copy of the singleton will make the unit tests fail. The unit tests have to go blindly in and assume that there are no offending constructs anywhere.

It might be a better idea to just have a smart_singleton::reset() which resets all the data stored in the singleton that the unit tests can call in the tear-down method.

---

If I was doing this, I'd actually make this a CRTP class so that it becomes very easy to create smart_singletons:

template<class T>
class smart_singleton
{
public:
    static std::shared_ptr<T> get_instance()
    {
        if (auto existing_instance = weak_instance.lock()) {
            return existing_instance;
        } else {
            std::shared_ptr<T> tmp_shared(new T());
            weak_instance = tmp_shared;
            return tmp_shared;
        }
    }
protected:
    smart_singleton() {}
private:
    smart_singleton(const smart_singleton<T>&) = delete;
    smart_singleton operator=(const smart_singleton<T>&) = delete;
    smart_singleton(smart_singleton<T>&&) = default;
    smart_singleton& operator=(smart_singleton<T>&&) = default;

    static std::weak_ptr<T> weak_instance;
};

template<class T>
std::weak_ptr<T> smart_singleton<T>::weak_instance;

Now to make a class a singleton, this is all I'd have to do:

class my_singleton : public smart_singleton<my_singleton>
{
    friend class smart_singleton<my_singleton>; // so that the smart-singleton can access the private constructor of this class
public:
    ...
private:
    my_singleton();
};

Answer 2

As I mentioned in the other answer, this solution for testing singletons is easy to use wrong. Any cached instance of the singleton makes your unit tests wrong. However, there is another way to make your singletons unit testing friendly: have the singleton implement an interface.

A real world example of this is Eclipse's Ecore. Ecore is a Java modeling framework which generates code for you. Each package it generates is equipped with a MyPackageFactory - effectively a singleton that you use with MyPackageFactory.eINSTANCE.createFoo(). However, MyPackageFactory is actually an interface; the singleton is implemented in MyPackageFactoryImpl. What this means is that you can inject the singleton into your classes, allowing you to mock for unit tests as well.

As a rough example:

my-singleton.h

#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();

    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

my-singleton.cpp

#include "my-singleton.h"
#include <iostream>

struct MySingletonImpl final : MySingleton {
    int value;

    void do_something() override {
        std::cout << value << '\n';
    }

    void set_something(int i) override {
        value = i;
    }
};

MySingleton &MySingleton::get_instance() {
    static MySingletonImpl instance;
    return instance;
}

It can then be used in a unit-testing friendly way like so:

uses-singleton.h

#pragma once

#include "my-singleton.h"

struct UsesSingleton {
    MySingleton *singleton;
    int i;

    explicit UsesSingleton(MySingleton &singleton, int i)
        : singleton{ &singleton }
        , i{ i }
    {}

    void do_something() {
        singleton->do_something();
        singleton->set_something(i);
        singleton->do_something();
    }
};

main.cpp

#include "uses-singleton.h"

int main() {
    UsesSingleton myStruct{ MySingleton::get_instance(), 10 };
    myStruct.do_something();
}

uses-singleton-test.cpp -- (using googlemock just as an example. also this is untested)

#include "gtest/gtest.h"
#include "gmock/gmock.h"

class MySingletonMock : public MySingleton {
public:
    MOCK_METHOD0(do_something, void());
    MOCK_METHOD1(set_something, void(int))
};

TEST(UsesSingletonTests, SampleTest) {
    MySingletonMock singleton;

    {
        ::testing::InSequence dummy;

        EXPECT_CALL(singleton, do_something());
        EXPECT_CALL(singleton, set_something(10));
        EXPECT_CALL(singleton, do_something());
    }

    UsesSingleton myStruct{ singleton, 10 };
    myStruct.do_something();
}

---

Do note that this will likely lead to a virtual table lookup at runtime, unless you instead declare the MySingletonImpl in the header file and define get_instance() in the header file:

my-singleton.h

#pragma once

class MySingleton {
public:
    static MySingleton &get_instance();

    // simply define an interface
    virtual void do_something() = 0;
    virtual void set_something(int i) = 0;
};

namespace detail {
    struct MySingletonImpl final : MySingleton {
        int value;

        void do_something() override;

        void set_something(int i) override;
    private:
        // We have to hide it now because it was previously
        // hidden by virtue of being in a cpp file
        MySingletonImpl() = default;
        friend MySingleton &MySingleton::get_instance();
    };
}

MySingleton &MySingleton::get_instance() {
    static detail::MySingletonImpl instance;
    return instance;
}

my-singleton.cpp

#include "my-singleton.h"
#include <iostream>

void detail::MySingletonImpl::do_something() {
    std::cout << value << '\n';
}

void detail::MySingletonImpl::set_something(int i) {
    value = i;
}

The compiler will then be likely to be able to optimize it to the correct function call, especially if get_instance was instead detail::MySingletonImpl &get_instance(), but I don't think that will cause any problems.

Either way, the performance penalty will probably be small, so it's probably not worth your time.