3
\$\begingroup\$

I'm working on some legacy code which uses the singleton pattern. The problem I have with the traditional singleton is that the instance lives until the program terminates. This is messing up the order in which things need to get destroyed and makes it difficult to write independent unit tests. Unfortunately, I can't get rid of the singleton by constructing a single instance and passing it around since this would break APIs.

I wrote the following wrapper around a class T that hands out shared pointers to a single instance of T. When the last shared pointer goes out of scope the object gets destroyed.

#include <memory>
#include <type_traits>
#include <mutex>

template <class T>
class refCountedSingleton
{
  static_assert(!std::is_default_constructible<T>::value, "T must have a private/protected constructor to ensure it can only be constructed by refCountedSingleton");
  static_assert(!std::is_copy_constructible<T>::value, "T must have a private/protected copy constructor to ensure instances handed out by refCountedSingleton cannot be copied");
  static_assert(!std::is_copy_assignable<T>::value, "T must have a private/protected copy assignment operator to ensure instances handed out by refCountedSingleton cannot be copied");
  static std::weak_ptr<T> p;
  static std::mutex mu;
public:
  static std::shared_ptr<T> getShared();
};

template <class T>
std::weak_ptr<T> refCountedSingleton<T>::p;
template <class T>
std::mutex refCountedSingleton<T>::mu;

template <class T>
std::shared_ptr<T> refCountedSingleton<T>::getShared()
{
    std::lock_guard<std::mutex> lock(mu);
    std::shared_ptr<T> temp = p.lock();
    if (!temp) {
      temp.reset(new T());
      p = temp;
    }
  return temp;
}

The following class could be used as T:

class obj {
  friend class refCountedSingleton<obj>;
private:
  obj() {}
  obj(obj const&);
  obj& operator=(obj const&);
};

Example use case:

{
  std::shared_ptr<obj> pm1 = refCountedSingleton<obj>::getShared();
  {
    // calling getShared() multiple times yields pointers to the same object
    std::shared_ptr<obj> pm2 = refCountedSingleton<obj>::getShared();
  }
} // instance of obj gets destroyed here
{
   // a new instance of obj gets greated
   std::shared_ptr<obj> pm3 = refCountedSingleton<obj>::getShared();
}

My singleton needs to be thread safe which is why I'm locking the mutex in getShared(). With the traditional singleton pattern performance can be improved by avoiding the expensive locking most of the time by leveraging double-checked locking. I'm wondering if something similar could be done here as well.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Correct me if I'm wrong, but as it stands the code in your question does not seem to compile (see yourself on godbolt). Seems like you're missing a friend declaration or something to that effect. Please verify and fix that, otherwise your question is off-topic for this site (help center). \$\endgroup\$ – AlexV Jul 1 at 6:25
  • 1
    \$\begingroup\$ The language guarantees (after C++11) that static storage duration objects are constructed in a thread safe manor. Which means the classic Singleton pattern works really well without any jumping through hoops: stackoverflow.com/a/1008289/14065 \$\endgroup\$ – Martin York Jul 1 at 8:48
  • \$\begingroup\$ Solving the order of destruction is relatively simple. Since the order of destruction is guaranteed based on the order of creation you simply have to force a specific order of creation. I shouw a technique to solve it here: stackoverflow.com/questions/335369/… \$\endgroup\$ – Martin York Jul 1 at 8:54
  • \$\begingroup\$ The problem with this design is that across libraries (static or shared) you can potentially get multiple instances of your singleton (so its not actually a singleton any more). The problem is the standard does not specify how libraries work so link this into libA.a and libB.a where both use a singleton for refCountedSingleton<XX> and you will have two instances of refCountedSingleton<XX>::p when these are compiled into your application. \$\endgroup\$ – Martin York Jul 1 at 9:01
  • 1
    \$\begingroup\$ Address things that don't work for you not shortcomings. Sure I can see that. The biggest issue with singleton is the lack of control of how it is created. Thus using the singleton pattern independently is usually an anti-pattern. It should be used in conjunction with another creator pattern so that you can adjust the behavior in non normal situations (like testing). I would suggest that you look at combining it with the factory pattern. By default you have a standard singleton but you can use the factory to install another type of singleton creator so that during testing it can create/destory \$\endgroup\$ – Martin York Jul 1 at 16:42
2
\$\begingroup\$
  1. While your code is thread-safe, it does not scale.
    Consider adding a fast-path in case there is an object.

  2. There is no need for a class, a free function is enough.

  3. std::make_shared() generally coalesces the two allocations (control-block and payload), making it significantly more efficient.
    If you want to ensure only get_shared_singleton() can ever construct a T, use a key.

class Key {
    Key() {}
    friend std::shared_ptr<T> get_shared_singleton();
};

template <class T>
std::shared_ptr<T> get_shared_singleton() {
    static std::atomic<std::weak_ptr<T>> p;
    auto r = p.load(std::memory_order_consume).lock();
    if (r) return r;
    static std::mutex m;
    std::lock_guard _(m);
    r = p.load(std::memory_order_consume).lock();
    if (r) return r;
    if constexpr (std::is_constructible_v<T, Key>)
        r = std::make_shared<T>(Key());
    else
        r = std::make_shared<T>();
    p.store(r, std::std::memory_order_release);
    return r;
}

Be aware it needs C++2a for std::atomic<std::weak_ptr<T>>.

Use it like:

struct X {
    X(X const&) = delete;
    X& operator=(X const&) = delete;
    X(Key /* optionally required for lockdown */) …
    …
};

…

get_shared_singleton<X>()
\$\endgroup\$
2
\$\begingroup\$

The problem with the above code is that it doing two things with one piece of code.

1: Singleton Creation. 2: Singelton LifeSpan.

I would separate these out into two separate classes.

The creation is done via an abstract factory (MySingletonFactory) but lifespan is controlled via explicit factory classes that are defined for the situation (so your code does not define the life span of the singleton).

The other issue of the above code is that I have seen this not working because of the way global variables are created in libraries. When these libraries are linked together it is not required to remove duplicate variables (as all variables were already resolved as much as possible during compilation). The linking phase will only try and resolve unresolved dependencies not all linkers will try and remove duplicated global variables (though I suppose this will depend on how sophisticated the linker is).

Here is how I would resolve the problem (by removing globals (using static storage function scope variables)).

Interfaces

#include <memory>
#include <iostream>
#include <cstdlib>

class MySingleInterface
{
    public:
        virtual ~MySingleInterface() {}

        virtual int doWork() = 0;
};
class MySingleFactoryInterface
{
    public:
        virtual ~MySingleFactoryInterface() {}
        virtual std::shared_ptr<MySingleInterface>  createTheObject() = 0;
};

Abstract Factory (I think?)

class MySingletonFactory
{
    private:
        friend class MySingleInterface;
        static MySingleFactoryInterface& getCurrentFactory(MySingleFactoryInterface* replace)
        {
            // Do not strictly need a default factory.
            // But it is nice to have one for standard (default)
            // operations. Then you only specify one in non standard
            // situations like testing. 
            static MyDefaultSingeltonFactory           defaultFactory;

            // The current factory we are using.
            static MySingleFactoryInterface*           currentFactory = &defaultFactory;
            if (replace)
            {
                std::swap(replace, currentFactory);
            }
            else
            {
                replace = currentFactory;
            }
            return *replace;
        }
    public:
        static std::shared_ptr<MySingleInterface> getInstance()
        {
            return getCurrentFactory(nullptr).createTheObject();
        }
        static MySingleFactoryInterface& setFactory(MySingleFactoryInterface& alternativeFactory)
        {
             return getCurrentFactory(&alternativeFactory);
        }
};

Example Default Singleton

class MyDefaultSingeltonFactory;
class MyDefaultSingleton: public MySingleInterface
{
    private:
        friend class MyDefaultSingeltonFactory;
        MyDefaultSingleton(){}
    public:
        virtual int doWork() override {static int value; return value++;}   // Normal operation.
};
class MyDefaultSingeltonFactory: public MySingleFactoryInterface
{
    // A Normal Singleton factory.
    // The singelton lives from creation until the end of the program.
    // Note: This is the behavior of this factory does not need to
    //       to be universal behavior.
    public:
        std::shared_ptr<MySingleInterface>  createTheObject() override
        {
            std::shared_ptr<MySingleInterface>  instance = std::shared_ptr<MySingleInterface>(new MyDefaultSingleton());
            return instance;
        }
};

Example Test Singelton

class MyTestSingleton: public MySingleInterface
{
    int value;
    public:
    MyTestSingleton()
        : value(rand())
    {}
    public:
        virtual int doWork() override {return value;}        // Operation for testing.
};

class MyTestSingletonFactory: public MySingleFactoryInterface
{
    // This factory creates a new instance each time.
    // Can be useful for some types of test.
    public:
        std::shared_ptr<MySingleInterface>  createTheObject() override
        {
            return std::make_shared<MyTestSingleton>();
        }
};

class MyTestPersistantSingletonFactory: public MySingleFactoryInterface
{
    // As long as the code holds a shared pointer it will be re-used.
    // Useful if you want to create the singleton during "setUp()" and
    // destroy it during "tearDown()" you get a brand new singleton for
    // each individual test.
    public:
        std::shared_ptr<MySingleInterface>  createTheObject() override
        {
            // I leave the exercise of locking to you.
            static std::weak_ptr<MySingleInterface>     persist;
            auto result = persist.lock();
            if (!result)
            {
                result = std::make_shared<MyTestSingleton>();
                persist = result;
            }
            return result;
        }
};

Test Harness

int main()
{
    srand(0);

    // Incrementing results.
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";

    // Each result indapendant.
    MyTestSingletonFactory  testFactory;
    MySingletonFactory::setFactory(testFactory);
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";
    std::cout << MySingletonFactory::getInstance()->doWork() << "\n";


    MyTestPersistantSingletonFactory    persistantFactory;
    MySingletonFactory::setFactory(persistantFactory);
    {
        // As long as we hold a reference then
        // we should get the same result.
        std::shared_ptr<MySingleInterface> local = MySingletonFactory::getInstance();
        std::cout << "L: " << local->doWork() << "\n";
        std::cout << "L: " << MySingletonFactory::getInstance()->doWork() << "\n";
        std::cout << "L: " << local->doWork() << "\n";
        std::cout << "L: " << MySingletonFactory::getInstance()->doWork() << "\n";
    }
    // No local reference so we will create a new value.
    std::cout << "N: " << MySingletonFactory::getInstance()->doWork() << "\n";
}

Output

> ./a.out
0
1
2
3
4
5
6
7
8
520932930
28925691
822784415
890459872
L: 145532761
L: 145532761
L: 145532761
L: 145532761
N: 2132723841
\$\endgroup\$
1
\$\begingroup\$

Although the motivation is to create independent unit tests, it only partially achieves that result.

Because the singleton is globally accessible from while it's alive, we've constrained the tests to run sequentially, which limits our ability to run more of them in a given time.

There may be benefit in making the singleton be thread-local for tests (this is an argument for using a suitable factory, as it can then be configured with a thread-local version for unit tests, and a global version for integration tests and production).

\$\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.