Qt QTcpSocket() readReady Signal never fires (slot never called) in multithreaded Server applicaton. waitForReadyRead() method works fine

Question

I'm writing a threaded TcpServer (each client in its own thread) using QTcpServer and QTcpSocket. The client application works correctly and sends data every 3 seconds but the readReady() signal never fires, meaning my receive_data() function is never called. When using socket->waitForReadyRead() and calling receive_data() by myself everything works fine. Please have a look at the code below, maybe I made some mistake with the moveToThread / connect functionality Qt offers.

Client.h

#ifndef CLIENT_H
#define CLIENT_H

#include <QThread>
#include <QTcpSocket>
#include <QHostAddress>
#include "PacketDefinitions.h"
#include "tcpserver.h"

class Client : public QObject
{
    Q_OBJECT
public:
    explicit Client(int socket,TcpServer *parent,bool auto_disconnect = true);
    ~Client();
    bool isGameServer(){return is_gameserver;}
    GameServerPacket getGameServerData(){return gameserver;}
    void run();
private:
    QTcpSocket* client;
    TcpServer *parent_server;
    int socket;
    GameServerPacket gameserver;
    ClientPacket clientdata;
    bool is_gameserver;
    bool auto_disconnect;
    QHostAddress client_ip;
    quint16 client_port;
signals:
    void disconnected(Client *);
private slots:
    void remove_from_clientlist();
    void receive_data();
    void display_error(QAbstractSocket::SocketError error);
};

#endif // CLIENT_H

Client.cpp

#include "client.h"
#include "PacketDefinitions.h"
#include "time.h"
#include <iostream>

Client::Client(int _socket, TcpServer *parent,bool _auto_disconnect)
{
    auto_disconnect = _auto_disconnect;
    parent_server = parent;
    is_gameserver = false;
    socket = _socket;
}

void Client::run(){ 
    client = new QTcpSocket();

    if(client->setSocketDescriptor(socket) == false){
        std::cout << client->errorString().toStdString() << std::endl;
        remove_from_clientlist();
        return;
    }

    connect(client,SIGNAL(disconnected()),this,SLOT(remove_from_clientlist()));
    if(connect(client,SIGNAL(readyRead()),this,SLOT(receive_data()),Qt::DirectConnection) == false) return;
    connect(client,SIGNAL(error(QAbstractSocket::SocketError)),this,SLOT(display_error(QAbstractSocket::SocketError)));

    client_ip = client->peerAddress();
    client_port = client->peerPort();

    std::cout << "New incomming connection " << client->peerAddress().toString().toStdString() << ":" << client->peerPort() << std::endl;

    //this works fine
//    while(client->waitForReadyRead()){
//        receive_data();
//    }
}

void Client::receive_data(){
       QDataStream stream(client);
       stream.setVersion(QDataStream::Qt_5_2);
       quint32 magic; stream >> magic;
       //interpret data
       if(magic == GAMESERVER_MAGIC){
           is_gameserver = true;
           gameserver.Read(stream);
           gameserver.port = client_port;
           gameserver.ip = client_ip;
           time(&(gameserver.last_update));
           parent_server->add_server(gameserver.ip.toString(),gameserver);
           std::cout << "GameServer " << gameserver.name << " registerd" << std::endl;
       }else if(magic == CLIENT_MAGIC){
           is_gameserver = false;
           clientdata.Read(stream);
           //get nearby servers
           GameServerListPacket server_list = parent_server->getServerList(clientdata);
           QDataStream outstream(client);
           server_list.Write(outstream);
           std::cout << "Sending ServerList(" << server_list.server_count << ") to " << client->peerAddress().toString().toStdString() << std::endl;
           if(auto_disconnect){
               //client->flush();
               client->waitForBytesWritten();
           }

       }else{
           std::cout << "Unknown package " << magic << std::endl;
       }

       //not enough data read, somthing is wrong, just for debugging
       if(client->bytesAvailable()> 0)  std::cout << "BytesAvailable " << client->bytesAvailable() << std::endl;

    if(auto_disconnect) remove_from_clientlist();//close the connection once the serverlist was deployed
}

In the TcpServer.cpp add_client() is called when newConnection() was emitted by the QTcpServer:

void TcpServer::add_client(){
    while(server->hasPendingConnections()){
        QTcpSocket *socket = 0;
        if(thread_pool.size() < max_connections && (socket = server->nextPendingConnection())){
            QThread *thread = new QThread();
            Client * client = new Client(socket->socketDescriptor(),this,auto_disconnect);
            client->moveToThread(thread);
            client->run();
            thread->start();
            connect(client,SIGNAL(disconnected(Client*)),this,SLOT(remove_client(Client*)));
            WRITELOCK(thread_pool.insert(client,thread));
        }
    }
}

the order calling client->run() and thread->start() doesn't seem to matter. Some time ago the code (not this exact code) worked fine but I can't remember what I changed that made it fail. Any help is appreciated!

Thanks in advance Fabian

Edit 1:

I derived from QTcpServer and reimplemented void incomingConnection(qintptr socketDescriptor) which works fine. I dont use QThreadPool, its just a QMap and remove_client(Client*) closes the QTcpSocket and stops the thread and removes it from the map. Everything works fine on linux, on windows I get the following error: QSocketNotifier: socket notifiers cannot be disabled from another thread ASSERT failure in QCoreApplication::sendEvent: "Cannot send events to objects owned by a different thread....

Caused by this remove_client(Client*)

void TcpServer::remove_client(Client *client){
    //disconnect(client,SIGNAL(disconnected(Client*)),this,SLOT(remove_client(Client*)));
    lock.lockForWrite();    
    QMap<Client*,QThread*>::iterator itr = thread_pool.find(client);
    if(itr != thread_pool.end()){
        //delete itr.key(); causes the problem on windows
        itr.value()->quit();
        itr.value()->wait();
        delete itr.value();
        thread_pool.erase(itr);
    }
    lock.unlock();
}

Where and how should I free the Client object? If i'd use QThreadPool theres no way to iterate through the clients in case i want to send a message to more than one client. I could use a list/map holding only the Client* but then QThreadPool might delete them for me right before i want to access it. Any suggestions?

Answer 1

There is a problem with how you move your client object to a new thread. Actually, Client::run executes in the same thread as TcpServer::add_client. Also QTcpSocket client remains in the default thread, while its container (Client class) is moved to a new thread. That's why the connection with Qt::DirectConnection type doesn't work. Try this:

class Client : public QObject
{
    Q_OBJECT
    ...
public slots:
    void run();
    ...
}

Client::Client(int _socket, TcpServer *parent,bool _auto_disconnect)
{
    ...
    client = new QTcpSocket(this);
}

void Client::run()
{ 
   ...
   connect(client, SIGNAL(readyRead()), this, SLOT(receive_data()));
   ...
}

And here's how you should move your client to a new thread:

void TcpServer::add_client()
{
    ...
    QThread *thread = new QThread();
    Client * client = new Client(socket->socketDescriptor(),this,auto_disconnect);
    client->moveToThread(thread);
    connect(thread, SIGNAL(started()), client, SLOT(run()));
    thread->start();
    ...
}

Answer 2

There are a number of things wrong with your code.

1.You have two QTcpSocket object trying to collect data from the same underlying socket descriptor. You appear to use the first one only to get access to the socket descriptor value which you then pass to your Client class. You might end up losing data because you won't be able to tell which socket will be getting what data from the operating system.

If you are creating a derived class of QTcpServer, rather reimplement QTcpServer::incomingConnection(qintptr socketDescriptor) instead of your existing TcpServer::add_client() function. Since this protected function is called once for every new connection, you don't need to make any connections to the newConnection() signal, nor do you have to loop while new connections are pending. You will also then only have one QTcpSocket connected to each socket descriptor so you won't lose data.

2.You seem to be using QThreadPool to manage threads. If you make Client a derived class of QRunnable (take not that with multiple inheritance of QObject, QObject must always be first), you don't need to check the maximum connections and you can eliminate all the QThread boiler-plating.

Taking 1. and 2. into account, your TcpServer::add_client() function will be replaced with:

void TcpServer::incomingConnection(qintptr socketDescriptor){
    Client * client = new Client(socketDescriptor,this,auto_disconnect);
    connect(client,SIGNAL(disconnected(Client*)),this,SLOT(remove_client(Client*)));
    QThreadPool::globalInstance()->start(client);
}

With QThreadPool, there's no need to check whether the max number of threads has been reached or not. If the maximum has been reached, any new calls to start() will queue the next connection until a thread becomes available.

3.The reason your socket is not reading any data unless you call waitForReadyRead() is because you're executing the run() function in the main thread, creating the local socket in the main thread, you make a DirectConnection with the instance of Client and then move client to a different thread. You cannot have direct connections between threads.

You will need to add a local QEventLoop to your run() function to handle all events and signals of your new thread but remember to connect signals to your loop's quit() slot so the run() function will exit, otherwise your thread will continue to run forever.