CODEWITHSUNDEEP
c++qt
Michael Bishara
Michael Bishara

Reputation: 3

C++ Singleton class creates more than one instance

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;
    }

Upvotes: 0

Views: 5251

Answers (6)

Kuba hasn't forgotten Monica
Kuba hasn't forgotten Monica

Reputation: 98495

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;
}

Upvotes: 2

Marek R
Marek R

Reputation: 38092

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.

Upvotes: -1

James Kanze
James Kanze

Reputation: 154007

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.

Upvotes: 1

Roy Iacob
Roy Iacob

Reputation: 412

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();

Upvotes: 0

Jerry YY Rain
Jerry YY Rain

Reputation: 4382

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;
    }
}

Upvotes: 4

Albi
Albi

Reputation: 320

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

Upvotes: -3

Related Questions