HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Unit-testing friendly singleton

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
singletonfriendlytestingunit

Problem

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 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 weak_instance;
};


smart_singleton.cpp

std::weak_ptr smart_singleton::weak_instance;

std::shared_ptr smart_singleton::get_instance()
{
    if (auto existing_instance = weak_instance.lock()) {
        return existing_instance;
    } else {
        std::shared_ptr 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.

Solution

I like the idea.

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

std::shared_ptr smart_singleton::get_instance()
{
    if (auto existing_instance = weak_instance.lock()) {
        return existing_instance;
    } else {
        std::shared_ptr 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::create_instance()
{
    if (weak_instance.lock()) {
        throw already_instantiated_error{}; // something better
    }
    std::shared_ptr tmp_shared(new smart_singleton());
    weak_instance = tmp_shared;
    return tmp_shared;
}

std::shared_ptr 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 `:

#ifndef SMART_SINGLETON_H_
#define SMART_SINGLETON_H_

#include 

...

#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 smart_singleton
{
public:
    static std::shared_ptr get_instance()
    {
        if (auto existing_instance = weak_instance.lock()) {
            return existing_instance;
        } else {
            std::shared_ptr tmp_shared(new T());
            weak_instance = tmp_shared;
            return tmp_shared;
        }
    }
protected:
    smart_singleton() {}
private:
    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 weak_instance;
};

template
std::weak_ptr smart_singleton::weak_instance;


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

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

Code Snippets

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;
    }
}
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();
}
#ifndef SMART_SINGLETON_H_
#define SMART_SINGLETON_H_

#include <memory>

...

#endif
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;
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();
};

Context

StackExchange Code Review Q#147506, answer score: 3

Revisions (0)

No revisions yet.