Multi-client TCP server with a thread for each client and DB connection pool

[don-prog]

I'm creating a Qt TCP (with SSL) server to which multiple clients should connect and use the DB, and this is the main task. I thought that I can use this architecture: each client has its own thread, and clients use DB via DB connection pool. Code of current architecture:

Client connection part

main.cpp:

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);

    MyServer server;

    server.listen(QHostAddress::Any, 1234);

    return a.exec();
}

myserver.h:

class MyServer : public QTcpServer
{
    Q_OBJECT

    explicit MyServer(QObject *parent = 0);
    ~MyServer();

protected:
    void incomingConnection(qintptr socketDescriptor) Q_DECL_OVERRIDE;

signals:
    void stopAll();
};

myserver.cpp:

MyServer::MyServer(QObject *parent) : QTcpServer(parent)
{
    //...
}

MyServer::~MyServer(){
    emit stopAll();
}

void MyServer::incomingConnection(qintptr socketDescriptor){
    QThread* clientThread = new QThread;
    MyClient *client = new MyClient(socketDescriptor, this);

    client->moveToThread(clientThread);

    connect(clientThread, SIGNAL(started()), client, SLOT(process()));

    connect(client, SIGNAL(finished()), clientThread, SLOT(quit()));
    connect(this, SIGNAL(stopAll()), client, SLOT(stopFromServer()));
    connect(client, SIGNAL(finished()), client, SLOT(deleteLater()));
    connect(clientThread, SIGNAL(finished()), clientThread, SLOT(deleteLater()));

    clientThread->start();
}

myclient.h:

class MyClient : public QObject
{
    Q_OBJECT
public:
    explicit MyClient(int socketDescriptor, MyServer *server);
    ~MyClient();

private:
    QSslSocket* socket = NULL;

public slots:
    void process();
}

myclient.cpp:

void MyClient::process(){
    //typical connection things
    connect(this->socket, SIGNAL(encrypted()), this, SLOT(ready()));
}

void MyClient::ready(){
    connect(socket,SIGNAL(readyRead()),this, SLOT(newData()));
}

---

DB connection pool part

DAO which will be executed in the client threads:

.cpp:

//DAO method

QList<QMap<QString, QString>> sqlResult = DatabaseService::executeQuery(sqlCommand);

//sqlResult processing

DatabaseService:

.h:

class DatabaseService
{
public:
    //...
    static QMap<QString, QString> executeQuery(QString command);

private:
    static QThreadStorage<QSqlDatabase> mDatabasePool;
    static QSqlDatabase getDatabase();

};

.cpp:

//...
QThreadStorage<QSqlDatabase> DatabaseService::mDatabasePool;

QSqlDatabase DatabaseService::getDatabase()
{
    if(DatabaseService::mDatabasePool.hasLocalData()) {
        return DatabaseService::mDatabasePool.localData();
    } else {
        auto database = QSqlDatabase::addDatabase("QPSQL", QUuid::createUuid().toString());
        database.setHostName("hostName");
        database.setDatabaseName("databaseName");
        database.setUserName("user");
        database.setPassword("password");
        database.open();
        DatabaseService::mDatabasePool.setLocalData(database);
        return database;
    }
}

QMap<QString,QString> DatabaseService::executeQuery(QString command){

    QSqlQuery query (DatabaseService::getDatabase());
    query.exec(command);

    //...

    return result;
}

//...

But after this answer on the CodeReview(please check the answer and discussion below) I think maybe I need to use LibEvent library.

So, I have two questions:

1. Should I use LibEvent to manage a large number of network connections and how can I do this? Or maybe I can use native Qt solutions for this?
2. How good is current architecture? What could be fixed and improved and how?

[VRonin]

1. the fact that Qt wants a QThread for each socket is a misconception and actually reduces performance compared to spreading the sockets among QThread::idealThreadCount() threads.
2. an object in the main thread that handles the db is a good idea but instead of a static method I would implement it async. You send a signal to the db manager with the query, it saves the results in a QVector<QVariantMap> (which is more appropriate for generic handling than QList<QMap<QString, QString>>) and sends them in another signal

What I would do is separate socket communication from processing. If all MyClient does is receive the data and pass the data along you don't even need multi-threading at all on the socket side

[don-prog]

@VRonin

1. Ok, so, if I will use threads number which will be equals to the QThread::idealThreadCount() and will spread MyClients equally between these threads(of course, I will use one main thread for the MyServer), then question about multithreading and managing network connections will be solved?

2. DB interaction is fine in the current architecture, but I also can use QtConcurrent::run for async, correct?

3. So, what about LibEvent? I think now I don't need it and I can use current architecture with the improvements above. Is it correct?

[VRonin]

1. Yes but you have to do it manually. in reality I would suggest you keep it all in 1 thread unless you have to run intense processing on the data you receive
2. not really as DBs are not really made for multi-threading and instances being kept around. Just handle it in the main thread.
   From http://doc.qt.io/qt-5/qsqldatabase.html:

QSqlDatabase is a value class. Changes made to a database connection via one instance of QSqlDatabase will affect other instances of QSqlDatabase that represent the same connection. Use cloneDatabase() to create an independent database connection based on an existing one.
Warning: It is highly recommended that you do not keep a copy of the QSqlDatabase around as a member of a class, as this will prevent the instance from being correctly cleaned up on shutdown. If you need to access an existing QSqlDatabase, it should be accessed with database(). If you chose to have a QSqlDatabase member variable, this needs to be deleted before the QCoreApplication instance is deleted, otherwise it may lead to undefined behavior.

3. LibEvent was suggested by somebody who clearly stated he did not know Qt. I would just ignore that advice

[don-prog]

@VRonin

1. "Yes but you have to do it manually" What do you mean?
2. In the previous comment you said that I need "to spreading the sockets among QThread::idealThreadCount() threads", but now you say that "I would suggest you keep it all in 1 thread", so, what exactly should I choose for the current architecture?
3. Ok, can you share a sample with this DB interaction architecture "You send a signal to the db manager with the query, it saves the results in a QVector<QVariantMap> and sends them in another signal"? Because I can't find good sample, but I need to see it in detail.
4. Is my current DB interaction architecture with database connections pool is fine if I will fix it only with using cloneDatabase() for instantiation and with using QSqlDatabase::database() for accesing DB?

Sorry for a lot of questions, and thanks!

[jsulm]

@don-prog
2. @VRonin is talking about database access which should be kept in one thread not sockets

[VRonin]

1. That you have to keep a record of how many sockets are on each thread and decide to send incoming connection to the thread that is less burdened
2. What I'm saying is that you are optimising prematurely. Try using async sockets in a single thread. in 90% of the cases the performance is acceptable and there is no need to spread them across threads at all

struct QueuedQuery
{
    quint64 queryID;
    QString query;
    QVariantList positionalBind;
    QVariantMap nameBind;
};
class QueryResult
{
public:
    QueryResult() = default;
    QueryResult(const QueryResult&) = default;
    QueryResult& operator=(const QueryResult&) = default;
    QVariant getResult(int record, int field) const { return m_value.value(record).value(field); }
    QVariant getResult(int record, const QString& field) const { return m_value.value(record).value(m_fields.indexOf(field)); }
    const QStringList& fields() const { return m_fields; }
private:
    QVector<QVector<QVariant> > m_value;
    QStringList m_fields;
    friend class dbManager;
}
Q_DECLARE_METATYPE(QueryResult) //You probably need to qRegisterMetaType it too
class dbManager : public QObject
{
    Q_OBJECT
    Q_DISABLE_COPY(dbManager)
public:
    dbManager(QObject* parent = Q_NULLPTR) :QObject(parent), m_autoStart(true) {}
    void enqueueQuery(const QueuedQuery& query) {/*Do some error checking maybe*/m_queryQueue.append(query); if (m_autoStart) processQueue(); }
    bool autoStart() const { return m_autoStart; }
    void setAutoStart(bool val) { m_autoStart = val; }
    Q_SLOT void processQueue()
    {
        while (!m_queryQueue.isEmpty()) {
            const QueuedQuery currQuery = m_queryQueue.dequeue();
            QSqlQuery queryToRun;
            queryToRun.prepare(currQuery.query);
            for (auto i = currQuery.positionalBind.constBegin(); i != currQuery.positionalBind.constEnd(); ++i)
                queryToRun.addBindValue(*i);
            for (auto i = currQuery.nameBind.constBegin(); i != currQuery.nameBind.constEnd(); ++i)
                queryToRun.bindValue(i.key(), i.value());
            if (!queryToRun.exec()) queryFailed(currQuery.queryID);
            bool firstRun = true;
            QueryResult tempRes;
            while (queryToRun.next()) {
                const auto tempRecord = queryToRun.record();
                const int filedCount = tempRecord.count();
                for (int i = 0; firstRun && i < filedCount; ++i)
                    tempRes.m_fields.append(tempRecord.field(i));
                firstRun = false;
                QVector<QVariant> recordResult;
                for (int i = 0; i < filedCount; ++i)
                    recordResult.append(tempRecord.value(i));
                tempRes.m_value.append(recordResult);
            }
            queryRun(currQuery.queryID, tempRes);
        }
        
    }
private:
    bool m_autoStart;
    QQueue<QueuedQuery> m_queryQueue;
signals:
    void queryRun(quint64, const QueryResult&);
    void queryFailed(quint64);
};

4. Thanks to the wise @kshegunov

whether an sql driver is reentrant (thus you can use different QSqlDatabase instances from different threads) is implementation specific.
I believe it's possible with pgsql
can't say for the others.

[don-prog]

@VRonin
Ok, almost done. Thanks for the provided sample, but for what now we need this design with query queue if we have dbManager and sockets in the same thread? Can we do this using usual function call?

[VRonin]

@don-prog Absolutely, if you have everything in the same thread feel free to use it in the usual way without complications