12
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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();
};
\$\endgroup\$
  • \$\begingroup\$ 1. Separated creation / getter I think i like this idea and will think about it. The main reason is that it allows you to assert that no instance exists in create();. Also it allows get() to return weak_ptr if you find it suitable in your particular scenario 2. ::reset() I do not like this one tho. When we get rid of singletons then we are left with only objects created localy in ut. This is by far more reliable then trusting others to call reset at the end of their test. \$\endgroup\$ – Marcin K. Nov 20 '16 at 15:48
  • \$\begingroup\$ 3. The idea with easy creating of new smart singletons is something I was already thinking about. Your CRTP method is very good but many people will claim that protected constructor instead of private is blasphemy. I don't know if there is any other way to do it tho. \$\endgroup\$ – Marcin K. Nov 20 '16 at 15:54
  • \$\begingroup\$ @MarcinKorn the constructor for the template must be protected so that the actual singleton can create its parent object. But it should never do anything. It's not the constructor for the singleton; the derived class's constructor is the singleton's constructor. You could name the template class something that makes it clearer that it is not of itself the singleton. \$\endgroup\$ – Justin Nov 20 '16 at 16:02
  • \$\begingroup\$ Oh you are right it's fine. As much as I knew that the real singleton is the derived one I was wrong regarding something else - for some reason I thought that this protected constructor could expose the already (correctly) implemented derived1 class to some erroneous behavior if you badly implement the derived2 like this: class derived2 : public smart_singleton<derived1>. But I gave it the 2nd thought and there should be no such risk. Thanks a lot this CRTP implementation is a great idea. \$\endgroup\$ – Marcin K. Nov 20 '16 at 17:53
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.