2
\$\begingroup\$

I'm currently trying to implement the singleton pattern in C++. After reading about it for a while now, it seems like there are a really large amount of different ways to do this.

Is the way I ended up doing it correct or did I miss out on something important here? Is this implementation thread safe in modern C++ (I read there were some changes to that in particular with C++11)? In addition I am not sure at all about the way I am accessing the class within MyFunction().

Is it really necessary to use a raw pointer there? Does this code contain any memory leaks?

#include <iostream>

class Singleton
{
private:

    Singleton();
    ~Singleton();  

public:

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

    void Test();
};

void Singleton::Test()
{
    std::cout << "Test() called" << std::endl;
}

Singleton::Singleton()
{
    std::cout << "CONSTRUCTOR CALLED" << std::endl;
}

Singleton::~Singleton()
{
    std::cout << "DESTRUCTOR CALLED" << std::endl;
}

void MyFunction()
{
    // use the singleton class
    Singleton * MySingleton = &Singleton::instance();   
    MySingleton->Test();    
}


int main()
{
    MyFunction();
    return 0;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I'd disable copyctor and = so that you won't accidentally copy the object. \$\endgroup\$ – Viacheslav Kroilov Jun 29 '18 at 12:40
  • \$\begingroup\$ BTW, it is perfectly thread-safe, because static variable initialization is thread-safe \$\endgroup\$ – Viacheslav Kroilov Jun 29 '18 at 12:41
4
\$\begingroup\$

Thread safety

Accessing the singleton is thread-safe. The change in C++11 has forced compilers to implement the construction of local static variables (like INSTANCE) in a thread-safe manner.

Note, however, that this doesn't make Singleton thread-safe under all circumstances: If multiple threads are calling Singleton::Test, I wouldn't be surprised to see some garbled output, as access to std::cout isn't synchronized.

Correctness

The current implementation violates the rule of 5: It has a custom destructor, but no custom copy and move constructors and assignment operators. And, to prevent making a copy of the singleton instance (or moving from it!), these 4 special functions should be deleted, like so:

public:
    Singleton(const Singleton&) = delete;
    Singleton(Singleton&&) = delete;
    Singleton& operator=(const Singleton&) = delete;
    Singleton& operator=(Singleton&&) = delete;

Note the public specifier, this ensures that these deletions are visible everywhere needed.

Access

While taking a pointer from the reference returned by Singleton::instance() is a valid use, it seems unnecessary in MyFunction as the pointer isn't required. Why not simply capture by a reference, like so:

auto& MySingleton = Singleton::instance();
MySingleton.Test();

Or if only one call is done:

Singleton::instance().Test();
\$\endgroup\$
  • \$\begingroup\$ Note, too, that many consider Singletons to be an anti-pattern and prefer using different approaches if feasible. \$\endgroup\$ – hoffmale Jun 29 '18 at 15:02
  • \$\begingroup\$ Kevlin Henney likes to note that it is ironic that programs never have exactly one singleton. They either have none or a bunch. I agree it is a mistake to start with “I need a singleton”. Rather, what is it that you really want to accomplish? That will help guide which form of (possibly singleton-like) behavior you want to implement. \$\endgroup\$ – JDługosz Jun 29 '18 at 17:29
  • \$\begingroup\$ Why the move constractor and move assignment should be deleted? Isn't it safe to move a singleton object? \$\endgroup\$ – Gupta Feb 13 at 10:19
  • \$\begingroup\$ @hsalimi the idea behind a Singleton is that there can only ever be one instance. Moving a Singleton instance means creating a new one. Assigning to a Singleton instance means that there is more than one instance in existence. Both operations violate the Singleton idea (plus you might get additional headaches if Singleton::instance() got moved from). \$\endgroup\$ – hoffmale Feb 13 at 11:12
  • \$\begingroup\$ @hoffmale I do not agree that moving a singleton object means creting a new one. IMHO, moving means just passing the internals of the singleton object to a new object or pointer. \$\endgroup\$ – Gupta Feb 13 at 13:26
1
\$\begingroup\$

You have created the classic Singleton pattern for C++.
https://stackoverflow.com/a/1008289/14065

This is great as a learning exercise. But in real life you can't use the Singleton like that because it tightly couples the objects to the things that use it. For instance testing your code now becomes exceeedingly difficult.

To use a s Singleton well (so it is not considered an anti pattern) you also need to combine the Singletons with another builder pattern (most will work but factory is easiest).
https://softwareengineering.stackexchange.com/a/40374/12917

You may have a default version of a Singleton that is run in production. But you should be able to configure the system so that at runtime the Singleton that is built at run-time is dependent on situation (so for testing you always insatiate an appropriate test Singleton).

Code Review:

Copying

You should prevent copying/moving of the object. If you allow copying it is no longer a Singleton, if you allow moving then it is it valid? Disable the copy/move constructors and assignment operators.

Usage

    // No reason to use a pointer.
    Singleton * MySingleton = &Singleton::instance();

    // When you have a pointer there is no ownership semantics
    // associated. So it is easy to make a mistake and accidentally
    // call delete on the pointer.


    // It returns a reference, so capture a reference
    Singleton&  MySingleton = Singleton::instance(); 

    // Here ownership semantics are clear.
    // You don't own the object (you simply have a reference to it).
    // It is your responsibility to make sure the object has not been
    // destroyed since you retrieved the reference.

    // Thus usually easier to use a Singleton as soon as you get it.
    Singleton::instance().Test();   // Don't need to keep a ref.
\$\endgroup\$
0
\$\begingroup\$

The way you have it will initialize the instance the first time it is used, if ever. That is handy for a component that may or may not be needed, and as an attempt to get dependencies between such systems automatically correct.

It will be destroyed during program shut down, after main returns (so, no memory leak), in the reverse order that such scoped static variables were created. If it gets used after that, you will have problems. Some singletons such as a logger may very well get used that way. You have no way to trap such bad uses, which is the downside of letting the compiler handle the details of creation and destruction.


⧺Enum.5 Don't use ALL_CAPS for enumerators and ⧺ES.9 Avoid ALL_CAPS names.

\$\endgroup\$
  • \$\begingroup\$ It is easy to solve the order of shut down issue. So that your Singleton is never used illegally. stackoverflow.com/a/335746/14065 \$\endgroup\$ – Martin York Jun 29 '18 at 18:19
  • \$\begingroup\$ Some arbitrary utility function logs something (a problem, or just chatter inserted while trying to find a bug) and it is called from here and there and anywhere, unrelated to the business-level objects at the top of the app. So how does controlling the order of creation and destruction of the systems help you? If anything runs after log has been destroyed, it might log something and Boom. \$\endgroup\$ – JDługosz Jun 29 '18 at 18:27
  • 1
    \$\begingroup\$ By controlling the order of destruction of the log (so that it is the last thing to be destroyed). You have solved the problem. If the log is the last thing to go nothing can use it after it has been destroyed. \$\endgroup\$ – Martin York Jun 29 '18 at 18:47
  • \$\begingroup\$ You can't control the total global order. The method linked will make it last among those wrapped local static variables that follow the protocol, but global scope variables will be destroyed after those. Note that "global ctor and dtor" is specifically a place where we want to log problems, since errors there just prevent loading or give mysterious crash on shutdown. \$\endgroup\$ – JDługosz Jun 30 '18 at 22:48
  • \$\begingroup\$ As I said it is easy to solve the problem. If you want to force logging to be destroyed after your component has logged follow the protocol. Then the log object will be destroyed after yours. You can't prevent errors but this problem is a solved problem. \$\endgroup\$ – Martin York Jul 1 '18 at 15:56

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.