Facing issue in making the classic Singleton implementation thread safe

Question

I was reading this-link that talks about making a singleton implementation thread safe.

Excerpt from the link:

Making the classic Singleton implementation thread safe is easy. Just acquire a lock before testing pInstance:

Singleton* Singleton::instance() {
Lock lock; // acquire lock (params omitted for simplicity)
if (pInstance == 0) {
pInstance = new Singleton;
}

return pInstance;
} // release lock (via Lock destructor)

As few things were omitted for simplicity in the above program, I thought I should write a sample code similar to what was suggested above.

This is what I wrote:

class Singleton {
public:
    static Singleton* instance();
    ~Singleton();
    void disp() { cout << "disp() called"<< endl; }
private:
    static std::mutex mu2;
    static Singleton* pInstance;
};
std::mutex Singleton::mu2;
Singleton* Singleton::pInstance = NULL;

Singleton* Singleton::instance() {
    mu2.lock();
    if (pInstance == 0) {
        pInstance = new Singleton;
    }
    return pInstance;
}

Singleton::~Singleton() {
    cout << "destructor called." << endl;
    mu2.unlock();
}

void f1() {
    Singleton *p = Singleton::instance();
    p->disp();
}
int main()
{
    std::thread t1(f1);
    t1.join();
    getchar();
    return 0;
}

Output:

disp() called

f:\dd\vctools\crt\crtw32\stdcpp\thr\mutex.c(51): mutex destroyed while busy

My question is:

1. What would be the implementation code of singleton class as per link's writer? without omitting anything?
2. Why my sample program is getting runtime error.
3. Is it possible to unlock the mutex in destructor?

I am running this code on Visual Studio.

Thank you.

Answer 1

To unlock a mutex, don't lock it by hand use std::unique_lock. Then it will be automatically unlocked after you leave a scope.

The way you do singleton with a pointer in global scope is asking for trouble at shutdown. Who's going to clean the singleton instance up? There isn't a delete anywhere.

Singleton* Singleton::instance() 
{
    std::unique_lock<std::mutex> lock{ mu2 }; // <=====
    if (pInstance == 0) {
        pInstance = new Singleton;
    }
    return pInstance;
}

// no unlock here, you only need to protect the creation.
Singleton::~Singleton() 
{
    cout << "destructor called." << endl;
}

void f1() 
{
    Singleton* p = Singleton::instance();
    p->disp();
}

The pattern you are looking for makes use of C++11 static initialization being threadsafe. And is described here : https://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton

static auto& get_instance()
{
    static YourClass instance;
    return instance;
}