Shared pointers to a singleton do not recognize each other

Question

I am currently learning how to use the C++11 smart pointers while programming a 2D game engine as a hobby using SDL. However, I ran into a problem while implementing an OOp wrapper for SDL.

The intent is to create a singleton class, which initializes SDL when it is constructed, and shuts SDL down when it gets destroyed. The singleton class has a static method getInstance that returns a shared_ptr to the singleton, and constructs the single instance if no instance exists, the idea being that all clients of the singleton own a shared_ptr to it, and when all clients get destroyed, the singleton is also destroyed. I do understand that singletons (and other globals) are generally bad, but I think that this could be one of the few cases where a singleton is appropriate, as there can only be one SDL library in use.

The problem lies in returning the shared_ptr from the getInstance method. Instead of using the same shared_ptr manager object, the shared_ptr instances are unrelated, and destroying a single one of them deallocates the singleton.

#include <iostream>
#include <memory>
using namespace std;

class Foo
{
public:
    ~Foo(){cout << "Foo <" << this << "> destroyed\n"; instance_ = nullptr;}
    static shared_ptr<Foo> getInstance()
    {
        if(instance_ == nullptr)
            instance_ = new Foo;
        //problem: the shared pointers created are unaware of each other
        return shared_ptr<Foo>(instance_);
    }
private:
    Foo(){cout << "Foo <" << this << "> constructed\n";}
    Foo(Foo& other){}
    void operator=(Foo& other){}
    static Foo* instance_;
};

Foo* Foo::instance_ = nullptr;

int main()
{
    shared_ptr<Foo> a = Foo::getInstance();
    shared_ptr<Foo> b = Foo::getInstance();
    shared_ptr<Foo> c = Foo::getInstance();
}

Output:

Foo <0x3e2a10> constructed
Foo <0x3e2a10> destroyed
Foo <0x3e2a10> destroyed
Foo <0x3e2a10> destroyed

@DyP Oh, same solution. I just wonder: Why didn't you made that an answer? — Daniel Frey, Oct 02 '13 at 22:12
@DanielFrey Because it doesn't explain where the OP's problem came from ;) — dyp, Oct 02 '13 at 22:13
`shared_ptr` is fundamentally wrong here. You can make it work but it’s still wrong: `shared_ptr` confers *shared ownership*. But a singleton doesn’t have shared ownership, it has *one* owner (the class’ context, e.g. a thread) and several consumers of that one owned instance. In other words: return a `weak_ptr` or a raw pointer. Or do it properly and *don’t* return a pointer but a reference. Using pointers here is useless. — Konrad Rudolph, Oct 02 '13 at 22:18
@KonradRudolph You are correct if you just look at it locally, but there might be a valid use-case: If you need a `shared_ptr` because some other method simply expects a `shared_ptr`. Yes, you could still add create a temporary `shared_ptr` with a null_deleter (or how that thing is called) but it might just be easier (and more efficient) to return a `shared_ptr` from `create`, avoiding superfluous allocations. — Daniel Frey, Oct 02 '13 at 22:29
The idea here is that clients of the singleton collectively own the single instance: For example, class Window instances require SDL to be initialized in order to work, so they request a shared pointer to the instance in their constructor. When all Windows and other subsystems get destroyed, SDL is no longer needed and can be shut down. So when Windows get destroyed, the shared pointers go out of scope, and when no windows remain, the singleton can be freed. — jms, Oct 02 '13 at 22:32
If my reasoning is incorrect, what approach would you reccomend to managing the initialization and shutting down of SDL automatically? — jms, Oct 02 '13 at 22:34
I think this is more like a shared resource than a singleton (because you're not using `getInstance` to access SDL). As you're already passing the `shared_ptr`, `getInstance` would only be required if there are multiple truly independent entities in your application that can request this resource. — dyp, Oct 02 '13 at 22:54
@user1062874 I updated my answer, see if the new version of the code is something that matches your use-case better... — Daniel Frey, Oct 02 '13 at 23:00
@user1062874 Since you want deletion as soon as all consumers go out of scope, a `shared_ptr` seems indeed appropriate. However, this is no longer a singleton: what happens when, after destruction, somebody else requests an instance? (Note that this isn’t necessarily a problem, I’m just pointing out that the “singleton” terminology can no longer be applied since it has a fixed contract that’s no longer fulfilled here.) — Konrad Rudolph, Oct 02 '13 at 23:07
@KonradRudolph Isn't that what is known as a Phoenix-singleton? (See my updated answer for a possible implementation) — Daniel Frey, Oct 02 '13 at 23:12

Daniel Frey · Accepted Answer · 2013-10-02T23:05:09.503

12

Your method could look like this:

static shared_ptr<Foo> getInstance()
{
    static std::shared_ptr<Foo> instance = std::make_shared<Foo>();
    return instance;
}

That way only a single, static instance is created (since C++11 this is also thread-safe) and you are always returning a copy from the static shared_ptr, which means all shared pointers that are returned share ownership.

Your original attempt created separate instances of shared_ptr from the same plain pointer, but this leads to detached ownership since the different shared pointers have no knowledge of each other and each one has its own internal "shared count".

Update: Re-reading your question I think you don't want to extend the life-time until the end of the program. Consider this method to destroy the instance as soon as all the returned shared pointers went out of scope:

static std::shared_ptr<Foo> getInstance()
{
    static std::weak_ptr<Foo> instance;
    static std::mutex mutex; // (a)
    const std::lock_guard< std::mutex > lock( mutex ); // (b)
    if( const auto result = instance.lock() ) return result;
    return ( instance = std::make_shared<Foo>() ).lock();
}

where you could delete the lines marked (a) and (b) if you are not in a multi-threaded environment.

Note that this is actually a so-called phoenix-singleton, meaning that if all shared pointers you received from getInstance() went out of scope and the instance was deleted, the next call to getInstance() will create another new instance of Foo.

edited Oct 02 '13 at 23:05

answered Oct 02 '13 at 22:10

Daniel Frey

55,810
13
122
180

I know the creation is thread-safe, but I wonder if you need an `atomic_load` for the `return` to be thread-safe, too. – dyp Oct 02 '13 at 22:18
If all shared pointers have gone out of scope and the singleton has been destroyed, is calling `getInstance()` guaranteed to instantiate a new singleton in all conditions? – jms Oct 02 '13 at 22:19
@user1062874 There's always an instance because of the local static variable. You could use a `weak_ptr` instead, and then had to enforce creation if it yields an empty `shared_ptr`. – dyp Oct 02 '13 at 22:20
@DyP An `atomic_load` is not necessary here, you can read `instance` from multiple threads simultaneously and even create copies. The shared count is already MT-safe. It would only be needed if `instance` could be *modified* in a MT environment, but that is not the case here. – Daniel Frey Oct 02 '13 at 22:22
Ah, the argument is probably Herb Sutter's famous *`const` means thread-safe*. I wondered because the Standard makes no mention of the word *thread* in the `shared_ptr` description itself. – dyp Oct 02 '13 at 22:27
Topic is old - but with the use of make_shared, g++ complains that the singleton constructor is private (as it should be in a singleton) – Vivian Miranda Jan 19 '16 at 19:09

score 2 · Answer 2 · answered Oct 02 '13 at 23:16

2

Rather than keeping a raw pointer in your static member, you should keep a weak_ptr. Use the lock() function to convert back to a shared_ptr; if it comes back empty then you need to allocate a new singleton object.

    static shared_ptr<Foo> getInstance()
    {
        shared_ptr<Foo> p = instance_.lock();
        if (!p)
        {
            p = new Foo;
            instance_ = p;
        }
        return p;
    }
private:
    static weak_ptr<Foo> instance_;

answered Oct 02 '13 at 23:16

Mark Ransom

299,747
42
398
622

@DanielFrey, true. I didn't see multi-threaded in the problem description. I also didn't see the edit you made while I was writing up my answer. – Mark Ransom Oct 02 '13 at 23:24

Shared pointers to a singleton do not recognize each other

2 Answers2

Linked