How to prevent name collisions when creating QSqlDatabase connection in multiple threads

Question

I have multi-threaded QTcpServer and for each database request, it creates new Thread to keep server Responsive. So in each thread I have to creating new QSqlDatabase connection. But I keep getting name collisions between connections.

here is my sample code to recreate issue.:-

#include <QSqlDatabase>

class DBTask
{
public:
    DBTask(ClientSocket *socket,ConnectionWorker *connectionWorker);
    ~DBTask();

    static void initStatic();    

private:

    static QThreadPool *pool; // all addConnection() call be be called in QtConcurrent::run with this pool
    static QString host, user, type, password, name;
    static quint64 dbConnectionNumber;

    QSqlDatabase db;

    ClientSocket *socket;
    ConnectionWorker *connectionWorker;

    bool addDatabase() ;    
};

quint64 DBTask::dbConnectionNumber=0;

DBTask::DBTask(ClientSocket *socket, ConnectionWorker *connectionWorker):
    socket(socket),
    connectionWorker(connectionWorker)
{
    dbConnectionNumber++;
}

bool DBTask::addDatabase() {

    QSqlDatabase db = QSqlDatabase::addDatabase(type,QString::number(dbConnectionNumber));
    db.setHostName(host);
    db.setDatabaseName(name);
    db.setUserName(user);
    db.setPassword(password);

    if(!db.open()){
        qWarning() << "Error while opening database for socket " << socket << '\n' << db.lastError();
        return false;
    }
    else {
        return true;
    }
}

this works fine when I manually check my application with GUI with human speed But when I run a c++ test code which simulates thousands of requests like this:-

void connectionTest(){

    QThreadPool pool;
    pool.setMaxThreadCount(10);

    for(int i=0;i<10;i++){
        QtConcurrent::run(&pool,[this](){
            for(int i=0;i<1000;i++){
                login(i%2); // login function sends request to QTcpServer
            }
        });
    }
}

I get multiple errors like this:-

QSqlDatabasePrivate::removeDatabase: connection '10' is still in use, all queries will cease to work.
QSqlDatabasePrivate::addDatabase: duplicate connection name '10', old connection removed.
QSqlDatabasePrivate::removeDatabase: connection '10' is still in use, all queries will cease to work.
QSqlDatabasePrivate::addDatabase: duplicate connection name '10', old connection removed.

and Server crashes with segfault

@Jarod42 getting same errors . of course connection names are different — LightSith, Feb 07 '20 at 22:08
There's one big problem with your question: It's not a [mcve]. With that, people are left to guess what each part does exactly, which is generally bad. Now, what is `DBTask::dbConnectionNumber`? If that is a static member, you have a classic case of a race condition when two instances of `DBTask` are created first and then their `addDatabase()` method is called. If that number was an atomic int, you could increment them atomically and store the result in a non-static (!) member to guarantee the uniqueness. BTW: Don't return `true` or `false` for errors, raise exceptions instead. — Ulrich Eckhardt, Feb 08 '20 at 08:46
@UlrichEckhardt sorry . I've added more code. btw I the answer given by zmb solved my problem. It wasn't working at first. he/she cleared that in his/her second comment — LightSith, Feb 08 '20 at 10:46

score 2 · Accepted Answer · answered Feb 07 '20 at 23:08

2

Even if you make the counter atomic, a thread can still get interrupted in the DBTask::addDatabase method (before creating the connection), another one can increment the counter and then they both continue and create 2 connections with the same id. You need to make both operations (increment the counter and the connection creation) in one transaction: inside the DBTask::addDatabase, by making use of a mutex lock.

answered Feb 07 '20 at 23:08

zmb

61
5

This will probably work, unless there is other code that accesses `addDatabase` as well and that code doesn't know anything about the mutex. – Ulrich Eckhardt Feb 08 '20 at 08:38
The addDatabase would own the mutex. The `addDabase()` will sequentially lock the mutex, increment counter & open a connection and then release the mutex. – zmb Feb 08 '20 at 09:08
@UlrichEckhardt Sorry, I meant that the `addDatabase` would incorporate the lock/realease code; the mutex would obviously be shared among all the threads. – zmb Feb 08 '20 at 09:19
Yes, but I meant `QSqlDatabase::addDatabase` to be clear. Other parts (perhaps even in a third-party library the author doesn't know about) could still access that function without coordination. However, I think that investigating in the direction I mentioned in my comment to the question should be done first, because there seems to be an obvious race condition. Whether it's resolved with a mutex or atomics is a matter of taste. – Ulrich Eckhardt Feb 08 '20 at 09:20
first it didn't worked. after reading you second comment, I added QMutex in addDatabase function and it is working – LightSith Feb 08 '20 at 10:16

score 0 · Answer 2 · answered Feb 08 '20 at 10:19

0

After adding QMutex to addDatabase, it works:-

bool DBTask::addDatabase() {

    mutex.lock();

    dbConnectionNumber++;
    db = QSqlDatabase::addDatabase(type,QString::number(dbConnectionNumber));

    mutex.unlock();
    ...
}

answered Feb 08 '20 at 10:19

LightSith

795
12
27

How to prevent name collisions when creating QSqlDatabase connection in multiple threads

2 Answers2