Singleton: is there a memory leak?

Question

This is a simple singleton:

class Singleton
{
    Singleton();
    virtual ~Singleton();

    Singleton * Singleton::getInstance() 
    {
        static Singleton * instance;

        if (!instance) {
            instance = new Singleton();
        };
        return instance;
    };
}

When main code calls Singleton::getInstance()->someMethod() for the first time, isn't the class instantiated twice? Will be there memory leak?

I am asking because Visual Leak Detector detects memory leak on the line with new Singleton().

Answer 1

When main code calls Singleton::getInstance()->someMethod() for the first time, isn't the class instantiated twice?

No. Calling a static member of Singleton does not instantiate Singleton, so the only instance of Singleton to exist here is the one you create with new. And you only create it once, because once instance points to it, you never call new again.

One problem you have here, however, is that you failed to make getInstance a static member function. I assume this is a typo/oversight since, otherwise, your program would not even compile. In fact, the declaration is ill-formed even as a non-static member. Furthermore, the constructor should be private to enforce the notion that only getInstance may instantiate the type.

Will be there memory leak?

Yes, and this is what Leak Detector is reporting. However, it's minimal: the problem is that there is nothing to delete that singleton instance right before your program shuts down.

Frankly I wouldn't worry about it. This may be one of those rare times that a leak is acceptable, mostly because more than a "leak" it's just a one-time failure to de-allocate on process shutdown.

However, if you get rid of the pointer entirely then you can avoid both problems at the same time, since one of the last things your program does will be to destroy objects of static storage duration:

#include <iostream>

class Singleton
{
public:
    ~Singleton()  { std::cout << "destruction!\n"; }

    static Singleton& getInstance() 
    {
        static Singleton instance;
        return instance;
    }

    void foo() { std::cout << "foo!\n"; }

private:
    Singleton() { std::cout << "construction!\n"; }
};

int main()
{
    Singleton::getInstance().foo();
}

// Output:
//   construction!
//   foo!
//   destruction!

(live demo)

No need even for a pointer!

This has the added benefit that the entire function becomes inherently thread-safe, at least as of C++11.

Answer 2

@LightnessRacesInOrbit answer is correct and to the point. Since your question is somehow dedicated to best practices when implementing a Singleton (which you should try to avoid btw), I'll give you my take on it. You can use something called the Curiously Recurring Template Pattern to create a Singleton base class which takes as a template parameter the class you want to make a Singleton. Once you've done that correctly, creating Singletons is like a walk in the park. Just derive Foo from Singleton<Foo> and make Foo::Foo() and Foo::~Foo() private. Here is the code I use, with comments, live on Coliru:

// Singleton pattern via CRTP (curiously recurring template pattern)
// thread safe in C++11 and later

#include <iostream>
#include <type_traits>

// generic Singleton via CRTP
template <typename T>
class Singleton
{
protected:
    Singleton(const Singleton&) = delete; // to prevent CASE 3
    Singleton& operator=(const Singleton&) = delete; // to prevent CASE 4
    Singleton() noexcept = default; // to allow creation of Singleton<Foo>
    // by the derived class Foo, since otherwise the (deleted)
    // copy constructor prevents the compiler from generating
    // a default constructor;
    // declared protected to prevent CASE 5
public:
    static T& get_instance()
    noexcept(std::is_nothrow_constructible<T>::value)
    {
        static T instance;
        return instance;
    }
    // thread local instance
    static thread_local T& get_thread_local_instance()
    noexcept(std::is_nothrow_constructible<T>::value)
    {
        static T instance;
        return instance;
    }
};

// specific Singleton instance
// use const if you want a const instance returned
// make the constructor and destructor private
class Foo: public Singleton</*const*/ Foo>
{
    // so Singleton<Foo> can access the constructor and destructor of Foo
    friend class Singleton<Foo>;
    Foo() // to prevent CASE 1
    {
        std::cout << "Foo::Foo() private constructor" << std::endl;
    }
    // OK to be private, since Singleton<Foo> is a friend and can invoke it
    ~Foo() // to prevent CASE 2
    {
        std::cout << "Foo::~Foo() private destructor" << std::endl;
    }
public:
    void say_hello()
    {
        std::cout << "\t Hello from Singleton" << std::endl;
    }
};

int main()
{
    Foo& sFoo = Foo::get_instance();
    sFoo.say_hello();

    Foo& sAnotherFoo = Foo::get_instance(); // OK, get the same instance
    sAnotherFoo.say_hello();

    Foo& sYetAnotherFoo = sFoo; // still OK, get the same instance
    sYetAnotherFoo.say_hello();

    thread_local Foo& stlFoo = Foo::get_thread_local_instance(); // thread local instance
    stlFoo.say_hello();

    // CASE 1: error: 'Foo::Foo()' is private
    // Foo foo;
    // Foo* psFoo = new Foo;

    // CASE 2: error: 'Foo::~Foo()' is private
    Foo* psFoo = &Foo::get_instance(); // can get pointer
    psFoo->say_hello();
    // delete psFoo; // cannot delete it

    // CASE 3: error: use of deleted function 'Foo::Foo(const Foo&)'
    // Foo foo(sFoo);

    // CASE 4: error: use of deleted function 'Foo& Foo::operator=(const Foo&)'
    // sFoo = sAnotherFoo;

    // CASE 5: error: 'Singleton<T>::Singleton() [with T = Foo]' is protected
    // Singleton<Foo> direct_sFoo;
}

Answer 3

new Singleton() does not have a matching delete, so yea, you are leaking a resource.

You'll get memory back when the program closes, but not all resources are returned when the program ends.

You can fix it by making instance a std::auto_ptr or std::unique_ptr. Or just don't use a pointer.