Thread Safe Singleton Segmentation Fault

[Peter_APIIT]

Hello to all, i try to implement a thread safe singleton but it get segmentation fault after main exit.

Below is my code:

Singleton Class
// =====================================
class ClsSqliteDatabaseManager : public ClsDatabaseManager
{
public:
    ~ClsSqliteDatabaseManager();

 static ClsSqliteDatabaseManager* CreateInstance();
 static ClsSqliteDatabaseManager* GetInstance();
 static void DestroyInstance(ClsConnectionPool&);

private:
    ClsSqliteDatabaseManager();

private:
    ClsConnectionPool m_ConnPool;
    IsNeutralMap m_IsNeutralMap;

    static mutex m_Mutex;
    static recursive_mutex m_RecurMutex;
    static condition_variable m_CondVar;

    static ClsSqliteDatabaseManager* m_DbManager;


};
// =====================================
Static Initialization in DBManager.cpp file

ClsSqliteDatabaseManager* ClsSqliteDatabaseManager::m_DbManager = 0;

mutex ClsSqliteDatabaseManager::m_Mutex;
recursive_mutex ClsSqliteDatabaseManager::m_RecurMutex;

condition_variable ClsSqliteDatabaseManager::m_CondVar;


Implementation
// ===========================================
ClsSqliteDatabaseManager* ClsSqliteDatabaseManager::CreateInstance()
{
    unique_lock<mutex> lock(m_Mutex);

    if (m_DbManager == 0)
    {
        m_DbManager = new ClsSqliteDatabaseManager();
        m_CondVar.notify_all();
    }

    return m_DbManager;
}

void ClsSqliteDatabaseManager::DestroyInstance(ClsConnectionPool& ConnPool)
{
    unique_lock<mutex> lock(m_Mutex);

    // Close all DB connection in the conn pool
    // This statement same with m_ConnPool.CloseAllConn();
    ConnPool.CloseAllConn();

    ConnPool.FreeSessionMemory();

    delete m_DbManager;
}

ClsSqliteDatabaseManager* ClsSqliteDatabaseManager::GetInstance()
{
    if (m_DbManager == 0)
    {
        unique_lock<mutex> lock(m_Mutex);

        m_CondVar.wait(lock);
    }

    return m_DbManager;
}
// =======================================
Main.cpp

int main()
{
    cout << "Hello World" << endl;
    Json::Value value;
    ClsDatabaseManager* dbManager = ClsSqliteDatabaseManager::CreateInstance();

    ClsSqliteDatabaseManager::DestroyInstance(dbManager->GetConnectionPool());

    return 0;  // Segmentation Fault Here

I try to debug using gdb and backtrace it shows on line return 0 in Main.cpp
I try to remove the static mutex in the singleton DB Manger class but it still casue segmentation fault.
Is it the local variable in single Db Manager class that cause this problem.

Please suggest a solution which thread safe and no segmentation fault.
Please help .

Thanks.

[D_Drmmr]

In GetInstance you access m_DbManager unsynchronized, so to make it thread safe, you should change it to.

ClsSqliteDatabaseManager* ClsSqliteDatabaseManager::GetInstance()
{
    unique_lock<mutex> lock(m_Mutex);
    while (m_DbManager == 0)
    {
        m_CondVar.wait(lock);
    }

    return m_DbManager;
}

However, that still leaves the possibility of a deadlock if CreateInstance is not called. IMO this design is difficult to use correctly.

It's not clear to me how DeleteInstance should be used. Why does it take a parameter?
Also, if you want to explicitly create and delete your singleton, why not create it before any worker threads are created and delete it after all worker threads have finished. That way you don't need any synchronization, at least not for accessing the singleton.

[Peter_APIIT]

In GetInstance you access m_DbManager unsynchronized, so to make it thread safe, you should change it to.

However, that still leaves the possibility of a deadlock if CreateInstance is not called. IMO this design is difficult to use correctly.

It's not clear to me how DeleteInstance should be used. Why does it take a parameter?
Also, if you want to explicitly create and delete your singleton, why not create it before any worker threads are created and delete it after all worker threads have finished. That way you don't need any synchronization, at least not for accessing the singleton.

First of all, thanks for your reply.
I will called the createInstance() method at main before any other class try to called the method of GetInstance();

As you said, my design is difficult to use it correctly. What are the correct approach to design a thread safe singleton class. I realized that double checked also is not thread safe.

The reason of DestroyInstance which accepts a parameter is because it is a static function and cannot access the member variable of the singleton class. Therefore, it accepts arguments and free some memory.

I hope i make it clear.

Thanks.

In regarding to the segmentation fault, what could be the reason which caused the error ?

Thanks again.

[D_Drmmr]

As you said, my design is difficult to use it correctly. What are the correct approach to design a thread safe singleton class. I realized that double checked also is not thread safe.

I said your design is difficult to use correctly, due to the fact that GetInstance will wait for CreateInstance to finish.
If you want explicit creation/deleting of your singleton, then consider if you need to provide a thread-safe interface, or whether it is enough to let the user of the class deal with that (as described in my previous post).
If you don't need explicit creation/deleting, but you do require a thread-safe interface, you could just implement the synchronization in the GetInstance function (and remove the Create/DeleteInstance functions).

ClsSqliteDatabaseManager* ClsSqliteDatabaseManager::GetInstance()
{
    unique_lock<mutex> lock(m_Mutex);
    if (m_DbManager == 0)
    {
        m_DbManager = new ClsSqliteDatabaseManager();
    }

    return m_DbManager;
}

You can register a function that will delete the instance by calling the atexit function.

The reason of DestroyInstance which accepts a parameter is because it is a static function and cannot access the member variable of the singleton class.

Then how are you accessing it in order to pass it to the function? Seems to me the DeleteInstance function can do that itself just as well.

In regarding to the segmentation fault, what could be the reason which caused the error ?

It could be caused by the way you implemented the DeleteInstance function. You didn't show the implementation of the constructor and destructor of the singleton class. Perhaps you free resources in the connection pool twice.

[Peter_APIIT]

Thanks for your reply. I do removed the create instance method and follow your advise.

I have solve the segmentation fault problem.

May i know what tools to test whether the software program was thread safe ?

Thanks.