QTcpServer handling multiple clients asynchronously

Kelvin Royal

Hello everyone. I have this weird problem and i don't know what is causing this. I wrote a server that listens on a port on my CentOS 7 VPS then do some little filtering then send the packet to another port (Squid listening on the latter). Everything works fine. But after some few hours running. It just stops receiving any connection....I thought it was crashing but after doing a quick "ps -e" or "htop" command i can see the program is still listed as running. Can anyone please tell me what am doing wrong? I have some theories though. After going through the documentation i notice QTcpServer has a max connection that defaults to 30. If i change that to a higher value will it solve my issue? What does this actually do? My second theory is the server block because its receive to many connections that the global threadpool can handle so it queues indefinitely. So maybe setting a higher threadpool number might fix this? Right now what i do is i restart the app every 4-5 hours. But its not pratical. I would like to fix this. Here is my code:

// main.cpp
#include <QCoreApplication>
#include "serverplus.h"

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

    ServerPlus srv;
    srv.startServer();

    return app.exec();
}

Server header implementation

#ifndef SERVERPLUS_H
#define SERVERPLUS_H

#include <QTcpServer>
#include "connectionhandler.h"

class ServerPlus : public QTcpServer
{
    Q_OBJECT
public:
    ServerPlus(quint16 srvPort = 1234, QObject *parent = 0);
    ~ServerPlus();

public slots:
    void startServer();

protected:
    void incomingConnection(qintptr socketDescriptor);

private:
    quint16 port;
};

#endif // SERVERPLUS_H

Server

#include "serverplus.h"

ServerPlus::ServerPlus(quint16 srvPort, QObject *parent) : QTcpServer(parent),
    port(srvPort)
{
}

void ServerPlus::startServer()
{
    if(!listen(QHostAddress::LocalHost, port)){
        deleteLater();
    }
}

void ServerPlus::incomingConnection(qintptr socketDescriptor)
{
    QPointer<connectionHandler> handle = new connectionHandler(socketDescriptor, port, this);
}

ServerPlus::~ServerPlus()
{
    if(isListening()){ close(); }
    exit(0);
}

// connectionHandler header file handles each connection asynchronously

#ifndef CONNECTIONHANDLER_H
#define CONNECTIONHANDLER_H

#include <QObject>
#include <QAbstractSocket>
#include <QHostAddress>
#include <QTcpSocket>
#include <QThreadPool>
//#include <QMutex>

#include "requesthandler.h"

class connectionHandler : public QObject
{
    Q_OBJECT

private:
    int socketDescriptor, bufferLimit = 16384;
    quint16 port, proxyPort = 1235; //Port running squid
    QByteArray packet, CRLF = "\r\n";
    QPointer<QTcpSocket> proxySocket, srvSocket;
    //QMutex mutex;

public:
    explicit connectionHandler(int descriptor, quint16 srvPort, QObject *parent = 0);
    ~connectionHandler();

    void setProxySocket();

    void setSocket();

public slots:
    void connected();

    void readyWrite();

    void readyRead();

    void error(QAbstractSocket::SocketError);

private slots:
    void sendRequest(QByteArray);

    void sendResponse(QTcpSocket *, QByteArray);
};

#endif // CONNECTIONHANDLER_H

//connectionHandler

#include "connectionhandler.h"

connectionHandler::connectionHandler(int descriptor, quint16 srvPort, QObject *parent): QObject(parent),
    socketDescriptor(descriptor), port(srvPort)
{
    setProxySocket();
    setSocket();
}

void connectionHandler::setProxySocket()
{
    proxySocket = new QTcpSocket(this);
    proxySocket->setReadBufferSize(bufferLimit);

    connect(proxySocket.data(), &QTcpSocket::connected, this, &connectionHandler::connected);
    connect(proxySocket.data(), &QTcpSocket::readyRead, this, &connectionHandler::readyWrite);
    connect(proxySocket.data(), static_cast<void (QTcpSocket::*) (QAbstractSocket::SocketError)>(&QAbstractSocket::error), this, &connectionHandler::error);

    proxySocket->connectToHost(QHostAddress::LocalHost, proxyPort);
}

void connectionHandler::setSocket()
{
    srvSocket = new QTcpSocket(this);
    srvSocket->setReadBufferSize(bufferLimit);

    connect(srvSocket.data(), &QTcpSocket::readyRead, this, &connectionHandler::readyRead);
    //connect(srvSocket.data(), &QTcpSocket::bytesWritten, this, &connectionHandler::bytesWritten);
    connect(srvSocket.data(), &QTcpSocket::destroyed, this, &connectionHandler::deleteLater);
    connect(srvSocket.data(), static_cast<void (QTcpSocket::*) (QAbstractSocket::SocketError)>(&QAbstractSocket::error), this, &connectionHandler::error);

    srvSocket->setSocketDescriptor(socketDescriptor);
}

void connectionHandler::connected(){
    if(!packet.isEmpty()){
        sendResponse(proxySocket.data(), packet);
    }
}

void connectionHandler::readyRead()
{
    request = srvSocket->read(srvSocket->bytesAvailable());

    if(!request.isEmpty()){
        requestHandler *handleRequest = new requestHandler(request);
        handleRequest->setAutoDelete(true);
        connect(handleRequest, &requestHandler::finished, this, &connectionHandler::sendRequest);
        QThreadPool::globalInstance()->start(handleRequest);
    }
}

void connectionHandler::readyWrite()
{
    response = proxySocket->read(proxySocket->bytesAvailable());

    if(!response.isEmpty()){
        sendResponse(srvSocket.data(), response);
    }
}

void connectionHandler::sendRequest(QByteArray req)
{
    packet = req;

    if(!proxySocket.isNull()){
        sendResponse(proxySocket.data(), packet);
        return;
    }

    //setProxySocket();
}

void connectionHandler::sendResponse(QTcpSocket *socketPtr, QByteArray resp)
{
    if(!resp.isEmpty() && socketPtr != nullptr && socketPtr->state() == QTcpSocket::ConnectedState){
        const char *ptr = resp.constData();
        int packetLength = resp.length();

        while(packetLength > 0 && socketPtr->isOpen()){
            if(bufferLimit > socketPtr->bytesToWrite()){
                int bytesSent = socketPtr->write(ptr, qMin(packetLength, bufferLimit));

                if(bytesSent == -1){
                    socketPtr->close();
                    socketPtr->deleteLater();
                }

                if(bytesSent > 0){
                    packetLength -= bytesSent;
                    ptr += bytesSent;
                }
            }else{ socketPtr->waitForBytesWritten(-1); }
        }
    }
}

void connectionHandler::error(QAbstractSocket::SocketError error)
{
        deleteLater();
}

connectionHandler::~connectionHandler()
{
    if(!proxySocket.isNull()){ proxySocket->deleteLater(); }

    if(!srvSocket.isNull()){ srvSocket->deleteLater(); }
}

EDIT: If i leave the app without restarting it. It starts working again after some minutes (10-30mins)

Thanks !!!

koahnig

@Kelvin-Royal

Hi and welcome to devnet forum

The number of 30 is for the number of maximum pending connections. This is IMHO the number of sockets allowed to wait for your handling.

As I see you are using QTcpServer than I do. QTcpServer is sending a newConnection() signal shich is typically connected to a slot for handling the new connections through nextPendingConnection with the help of hasPendingConnections. When the handling of new connections will take a moment or the app is busy for another reason new connections are put on stack/list. This list has a default length of 30.

You overwrite incomingConnection which probably may be also a way to do it, but personally I would follow the signal slot mechanism. Anyway to me it looks like you are side-cutting the orginal functionality which may cause your trouble.
There is a note in the documentation which might give you the hint:
Note: If another socket is created in the reimplementation of this method, it needs to be added to the Pending Connections mechanism by calling addPendingConnection().

However, there is another note as well. You need to check those carefully.

Also there is an example given as best use case.

kshegunov

Hello @Kelvin-Royal,
There are multiple and serious issues with your code.

For one all the sockets are in the main thread, there's no multithreading TCP here, because you didn't follow the documentation precisely. The ServerPlus::incomingConnection method is called from the thread where the QTcpServer resides (main() in this case). In which override you create an object in whose constructor the QTcpSocket instance is created, which is what the default implementation (i.e. nextPendingConnection()) does. If you want the socket to be in another thread, then you must transfer the descriptor (as per the documentation) to the thread that will handle the socket and only then create the socket object from that descriptor.
If in fact you didn't want to thread the sockets themselves then just use nextPendingConnection().

Secondly, you never actually delete your connectionHandler instances. This means all such objects are left hanging in memory and all sockets that are held by the mentioned objects are left open until the peer terminates the connection. Furthermore, even when the peer disconnects the objects are never cleaned up, they're left in the QObject tree to be freed on destruction of the TCP server object.

And finally, please explain the rationale behind calling ::exit() here? Did you want to make sure that the QCoreApplication destructor isn't called??!

ServerPlus::~ServerPlus()
{
    // ...
    exit(0);
}

Kind regards.

Kelvin Royal

@koahnig Thank you very much. I will implement the signals and slot way of doing it. It doesnt seem too difficult. Thanks for the insight. Cheers!!!

Kelvin Royal

@kshegunov Thank you for your remarks. But i thought QTcpServer & QTcpSocket work asynchronously? Which led me to think i do not need to spawn a thread for each client. And from the little research i made this is bad design because for say 100 clients you have 100 threads which will be pretty expensive IMHO. But please enlighten me more. I will like to understand this whole thing better.....As for the connectionHandler i actually delete it from the object itself when there is an error on the srvSocket. Please check the signals and slots, i connected it there. Or am doing it wrong? And for the exit my bad. I didnt really think about it. Thanks for bringing it up.

herlarbee

Why don't you watch VoidRealms video tutorials on youtube via https://www.youtube.com/playlist?list=PL2D1942A4688E9D63.
With emphasis on videos 63-70 as well as videos 135-141.
I believe he did a good job explaining the concept of threading efficiently and asynchronous designs in TCPServers.
You can also visit his website at voidrealms.com to get some source code to better understand the concepts of TCPServers.
Best of luck

kshegunov

@Kelvin-Royal

But i thought QTcpServer & QTcpSocket work asynchronously?

That is correct, they do.

Which led me to think i do not need to spawn a thread for each client.

Correct again, you don't need to. But if you're not going to, then there's no point in overriding the incomingConnection() method. Just use nextPendingConnection() in a slot connected to the newConnection() signal, like @koahnig suggested (a good example is present in the docs).

And from the little research i made this is bad design because for say 100 clients you have 100 threads which will be pretty expensive IMHO.

It depends on many things, but yes, ordinarily there wouldn't be much point in starting a separate thread for each connection. Sometimes when dealing with huge throughput a brokerage is employed (broker pattern). In any case there is a variety of approaches that are more or less suitable for different cases.

As for the connectionHandler i actually delete it from the object itself when there is an error on the srvSocket.

And when there's no error (which there isn't) the connection stays forever. Just connect the disconnected() signal from both sockets to the deleteLater() slot of the connectionHandler object and you should be good to go.

Or am doing it wrong?

No, not really, it just wasn't clear if you wanted to thread the sockets. The design is sound for the most part.

Kind regards.

Kelvin Royal

@kshegunov thank you very much really appreciate you taking your time to explain all this to me. And your are right. as soon as i connected both disconnect signals to the deleteLater() and went down the signals and slots road all the problems vanished. Thank you really. Because i had alot of time to spare i also followed @herlarbee suggestion(even though i watched this videos already) then decided to write a seperate server using QThreadPool approach seen on in the video title "Qt TCP Server design revisited". Then i benchmarked both. This new server after doing some benchmark test using ab tool in apache seems to outperform my previous solutions and handles alot more connections faster. Right now am very happy. When asking this question i just had 200 client limit in my brain but i got solutions that go way beyond that. Thank you guys.

Cheers !!!