What is wrong with this Singleton implementation?

Question

The idea is to have a Singleton in C++ deleted when the program ends. We learned this method of implementation in class:

class Singleton
{

private:
    static Singleton* the_singleton;

protected:
    Singleton()
    {
        static Keeper keeper(this);
        /*CONSTRUCTION CODE*/
    }
    virtual ~Singleton()
    {
        /*DESTRUCTION CODE*/
    }

public:
    class Keeper
    {

    private:
        Singleton* m_logger;

    public:
        Keeper(Singleton* logger):m_logger(logger){}

        ~Keeper()
        {
            delete m_logger;
        }
    };
    friend class Singleton::Keeper;

    static Singleton* GetInstance();
    {
        if (!the_singleton)
            the_singleton = new Singleton();
        return the_singleton;
    }
};

Singleton* Singleton::the_singleton = NULL;

The idea is that on the first time the Singleton is created, a static Keeper object will be created in the Singleton's C'tor, and once the program ends, that Keeper will be destroyed, and in turn will destroy the Singleton's instance it is pointing to.

Now, this method seems quite cumbersome to me, so I suggested to ditch the keeper class and make the Singleton's instance a static object of the getInstance method:

<!-- language: c++ -->

class Singleton
{

protected:
    Singleton()
    {
        /*CONSTRUCTION CODE*/
    }

    ~Singleton()
    {
        /*DESTRUCTION CODE*/
    }

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

    /*OBJECT FUNCTIONALITY*/
};

That way, the Singleton is constructed on the first call to the getInstance method, and destroyed once the program ends. No need for that keeper class.

I tested it and it worked just fine - the Singleton was created and destroyed at the right places. However, my TA said that this pattern is wrong, though he couldn't recall what exactly was the problem with it. So I'm hoping someone here has encountered this implementation before and can tell me what's wrong with it.

Answer 1

It's wrong, but your modifications didn't introduce the problems -- they've been there all along.

Right now there's no enforcement of the singleton property. The constructor needs to be private, not protected, in order to ensure additional instances aren't created.

---

In addition to that, the original code is completely unusable in a multithreaded scenario. Yours will work beginning with C++0x.

---

If the TA's keeper object had reset the global pointer back to NULL, his implementation would be able (in a single threaded environment) to recreate the singleton if needed during program cleanup. Read Singleton pattern in C++ . But this still would be a new instance of the singleton, which is almost as unexpected as using the singleton after its destructor has been called.

However, he neglected to do that, so his will happily use an invalid pointer during program shutdown after the keeper deletes the singleton. In yours, at least the memory region continues to be valid even if the object's lifetime has ended.

---

You will of course be marked down for using the improved version, this simply shows that the aim of the class is not to teach you C++, but the TA's misconceptions about C++.

Answer 2

There's no problem with it at all. The "Keeper" thing is nuts for this application, IMHO.

There are a few cases where it might be warranted. For instance, if the Singleton must take constructor arguments, allocating it statically might not be possible. Or if its constructor might fail, the simpler approach wouldn't allow you to recover or retry. So in some circumstances, doing something more complex might be needed. In many cases, though, it's unnecessary.