C++ Singleton class creates more than one instance

Question

The static method GetUI is called with mouse events, however noticed from debug that the constructor is called twice in some very rare cases with rush of mouse events.

The question is it the scheduler that halted in the middle of the construction, switching to another task process call that also started to create another instance ?

    Object* Interface::instance = 0;

    Object* Interface::GetUI() {
        if (instance == 0) {
            instance = new Interface();
        }
        return instance;
    }

Look up "Meyer's Singleton". And then learn not to use singletons. — juanchopanza, May 21 '14 at 08:56
If you're using Qt, I think there are some macros in the library to create thread-safe singletons. I think `Q_GLOBAL_STATIC` is what you're looking for, but I don't know if that's public (stable) API. — Stefan Majewsky, May 21 '14 at 09:31
you are writing about mouse events so this shouldn't be a multithreading issue, evrything should happen on main thread. Add assertion `Q_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread())` to make sure that this is a problem (if it fails check call stack). I recommend also disable assign operator and copy constructor. — Marek R, May 21 '14 at 10:27
@MarekR He hasn't mentioned `QT`, but it's a good point anyway. I would expect mouse events to all occur in the same thread. (Of course, he may be dispatching them from the main thread to other threads, in which case, the problem could be threading.) — James Kanze, May 21 '14 at 10:40

score 4 · Accepted Answer · answered May 21 '14 at 09:00

4

You should actually lock the singleton, otherwise, when multithreading, you will create multiple instances.
for C++11, you can use it as below.

#include <mutex>

class Singleton 
{
    static Singleton *singletonInstance;
    Singleton() {}
    static std::mutex m_;

    public:

    static Singleton* getSingletonInstance()
    {
        std::lock_guard<std::mutex> lock(m_);
        if(singletonInstance == nullptr)
        {
            singletonInstance = new Singleton();
        }
        return singletonInstance;
    }
}

answered May 21 '14 at 09:00

Jerry YY Rain

4,134
7
35
52

This defeats one of the goals of a `Singleton`, since it can result in order of initialization issues. (I doubt that it will be a problem in his case, however; if the singleton is only used when handling mouse events, it won't be called before static initialization have finished.) – James Kanze May 21 '14 at 09:03
@JamesKanze How does it result in "order of initialization issues"? The use of `std::mutex` as shown is IIRC perfectly kosher. This code is guaranteed to set `singletonInstance` with the address of the newly constructed `Singleton` exactly once. About the only dubious thing might be a missing memory fence, but presumably that's implied in the release of the mutex. That's really about the only issue that stands out to me. – Kuba hasn't forgotten Monica May 22 '14 at 15:41
why aren't you unlocking the mutex? – pkthapa Jan 30 '19 at 02:18

score 2 · Answer 2 · answered May 22 '14 at 15:30

The behavior you describe can only appear when the GetUI is used from multiple threads. I hope you are aware that you can't do any direct calls on GUI methods without proper locking or using the queued method invocation.

The thread-safe, idiomatic way of creating global variables in Qt is shown below. There's really no reason for a yet another implementation of it. Sometimes NIH is bad.

#include <QtGlobal>

class Object {};
class Interface {
public:
   static Object * GetUI();
};

// This must be in a *single* source file *only*. Not in header!
Q_GLOBAL_STATIC(Object, interfaceInstance)

Object* Interface::GetUI() {
    return interfaceInstance;
}

score 1 · Answer 3 · answered May 21 '14 at 10:06

The problem is that you have a race condition between the creation and the instantiation of the object. There are two possible solutions; you can synchronize the GetUI function, as Jerry YY suggested, or you can ensure that the singleton is created before you start threading; a race condition only occurs when you modify the object in at least one thread, and once you've created the object, instance is never modified.

One way of doing this is to simply define Interface::instance as:

Object* Interface::instance = Interface::GetUI();

Zero initialization ensures that before Interface::GetUI is called, Interface::instance is initialized to the null pointer, and the initialization of static objects takes place before main.

Otherwise: if you're sure that Interface::GetUI will never be called before entering main (which seems likely, given what you've said—there shouldn't be any mouse events before you enter main), then you can drop the test (or replace it with assert( instance != nullptr ) in Interface::GetUI, and just write:

Object* Interface::instance = new Interface();

Much simpler, and avoids all of the problems.

Can you point to where in the standard it's claimed that zero initialization happens for such objects? I've always thought (perhaps erroneously) that it's not guaranteed for explicitly initizalized objects - heck, it'd be a tad wasteful, indeed. — Kuba hasn't forgotten Monica, May 22 '14 at 15:38

score 0 · Answer 4 · answered May 21 '14 at 09:04

Here's a nicer singleton implementation with templating:

template <class T>
class Singleton 
{
private:
  static T * _instance;
protected:
  Singleton (){}
public:
  static T & get()
  {
    if (_instance)
    {
      return *_instance;
    }
    _instance = new T();
    return *_instance;
  }
};

template <class T>  T* Singleton<T>::_instance = 0;

Now implement it with inheritance:

class Foo : public Singleton<Foo>

And utilize it anywhere:

Foo::get().DoSomething();

score -1 · Answer 5 · edited May 23 '17 at 12:08

-1

Here is faster version of earlier solution (reduces use of mutex overhead).

#include <mutex>

class Singleton 
{
    static volatile Singleton * singletonInstance;
    Singleton() {}
    static std::mutex m_;

    public:

    static Singleton* getSingletonInstance()
    {
        if(singletonInstance == nullptr)
        {
            std::lock_guard<std::mutex> lock(m_);
            if(singletonInstance == nullptr) {
                Singleton *tmp = new Singleton(); // fight with compiler see link in comment
                singletonInstance = tmp;
            }
        }
        return singletonInstance;
    }
}

Anyway I'm not convinced that multithreading is a problem, since you've wrote about mouse events and this should come only from main thread. Add assertion Q_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread()) to make sure that this is a problem with synchronization (if it fails check a call stack).

I recommend also disable assign operator and copy constructor.

edited May 23 '17 at 12:08

Community

1
1

answered May 21 '14 at 10:13

Marek R

32,568
6
55
140

It also doesn't work. If any thread modifies an object, then no thread may access it without synchronization. You might want to read http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf. – James Kanze May 21 '14 at 10:22
thanks for link I have to read why this `tmp` variable is needed. – Marek R May 21 '14 at 10:32
Not only the `tmp` variable, but memory barriers or fences. (Alternatively, with C++11, I think making `instance` atomic should be sufficient.) – James Kanze May 21 '14 at 10:38
The `singletonInstance` may be non-null yet not fully written into. Say only 32 bits out of a 64 bit value may be written. I find attempts at reinventing this highly suspicious. The solution offered by Qt itself is tested and known to work. Why reinvent the wheel? – Kuba hasn't forgotten Monica May 22 '14 at 15:33

score -3 · Answer 6 · answered May 21 '14 at 08:57

-3

Your instance and your GetUI() method must be static.

answered May 21 '14 at 08:57

Albi

310
2
13

The instance and GetUI() are static. – Michael Bishara May 21 '14 at 09:00

C++ Singleton class creates more than one instance

6 Answers6