QTcpSocket never has data avaiable to read!

[sisco]

Hi there,

I'm having a problem with reading an incomming connection on my QTcpServer.

i create a QtcpServer object and connect the signal newconnection() to a custom made slot:


serv = new QTcpServer();
connect(serv, SIGNAL(newConnection()), this, SLOT(newsession()));
serv->listen(QHostAddress::Any, port);

then from the slot for each pending connection i create a thread to handle the incomming data:

void clienthandler::newsession()
{
QThread *t;
incomingslientsession *client;
while(serv->hasPendingConnections()){
t = new QThread();
client = new incomingslientsession(&db, serv->nextPendingConnection()->socketDescriptor(), &mutex);
connect(t, SIGNAL(started()), client, SLOT(dowork()));
connect(t, SIGNAL(finished()), t, SLOT(deleteLater())); // Delete thread after session is handeled
client->moveToThread(t);
t->start();
}
}
In the thread i create a new Qtcpsocket() and assign the given socketdescriptor. After that i try to read 20 bytes from the incomming stream (sending node sends 1000+ bytes)


QTcpSocket sock;
sock.setSocketDescriptor(socketdescriptor);

// Test code to read some bytes
int bytesRead = 0;
QByteArray buff;
while (bytesRead < 20) // First 20 bytes
bytesRead = sock.read(buff.data(), 20-bytesRead);

qDebug() << buff;

This is where the problem is, the socket never receives any bytes so this code gets stuck in an endless loop! i can see the data coming in when looking with wireshark and the socketdescriptor is also the right one.

Can anyone see the problem here?

Thanks,

Sisco

[wysota]

You are creating a local variable that goes out of scope immediately. Furthermore, the fact that a connection is established, doesn't mean there is data available on the socket.

[sisco]

The local variable is created inside the function where i immediatly use it to read the socket. here is the entire function, as far as i know there shouldn't be a problem with the way i create the variable and it should exist until the function has finished (never since the socket never has any bytes to read). The loop will run until a total of 20 bytes have been read from the socket, if no bytes are available it will just wait until there are.


void incomingclientsession::dowork()
{
qDebug("Session handler thread id: %d", (int) QThread::currentThreadId());

QTcpSocket sock;
sock.setSocketDescriptor(socketdescriptor);

QByteArray buff;
while (buff.length() < 20) // First 20 bytes
buff += sock.read(20-buff.length());

qDebug() << buff;
}

Thank you,

Sisco

EDIT: fixed an error in the loop

[wysota]

The local variable is created inside the function where i immediatly use it to read the socket. here is the entire function, as far as i know there shouldn't be a problem with the way i create the variable and it should exist until the function has finished (never since the socket never has any bytes to read).
As I said, there is no data to be read yet. And even if you sleep for 2 centuries inside that loop, there won't be any data available because you don't let Qt deliver any data to your socket.

[sisco]

Look like i have found the problem.

The error wasn't the loop waiting for data but it was passing the descriptor to the thread object. I changed the constructor from taking the descriptor of the socket to actually taking the pointer of the socket.

from:

incomingslientsession::incomingslientsession(QSqlD atabase *db, int socketdescriptor, QMutex *mutex)
{
this->db = db;
this->mutex = mutex;
this->socketdescriptor = socketdescriptor;
}

to:

incomingslientsession::incomingslientsession(QSqlD atabase *db, QTcpSocket *sock, QMutex *mutex)
{
this->db = db;
this->mutex = mutex;
this->sock = sock;
};

Now i can read from the socket, but to write data i have to create another new socket with the socketdescriptor given by the earlier socket.


while (sock->bytesAvailable() < 20) { // First 20 header bytes
if (!sock->waitForReadyRead()) {
qDebug("Disconnected");
qDebug() << sock->error();
return;
}
}

buff += sock->readAll();
qDebug() << buff;

QTcpSocket writesock;
writesock.setSocketDescriptor(sock->socketDescriptor());
sock->write("AAAA");
sock->waitForBytesWritten();

Can anyone explain? :confused:

Sisco

[wysota]

What happens if you try to write to "sock"?

By the way, in the code you posted you are doing at least two or three things wrong that sooner or later will backfire at you (I think one of them already had).

[sisco]

Hello again,

I have been digging trough the QT documentation and found the problem. since i was using the standard Qtcpserver class with each new connection i used QTcpServer::nextPendingConnection which gives a QTcpSocket pointer. Now when setting a socketdescriptor the doc gives:
Note: It is not possible to initialize two abstract sockets with the same native socket descriptor.

so when opening a connection i used this:

QTcpSocket *sock = server->nextPendingConnection

and started another thread passing the descriptor of the socket:

client = new incomingclientsession(&db, sock->socketDescriptor(), &mutex);

In the thread i was creating another QTcpSocket and set the descriptor to the one that was given, This was probably the cause of not being able to read the socket.

My current code look like this:

Server class derives from QTcpServer where i override some functions:


class tcpserver : public QTcpServer
{
Q_OBJECT

public:
tcpserver();
int nextConnection();
bool hasPendingConnections(); // Override

private:
QList<int> connections; // List to store pending connections (not sure if this is the right way)

protected:
void incomingConnection(int handle); // Override
};

The incommingConnection function will store the connection in the list and emit the newConnection signal instead of creating a QTcpSocket and adding that to the list.


void tcpserver::incomingConnection(int handle)
{
connections.append(handle);
emit this->newConnection();
}

with this i can connect the signal and slot like this:

connect(serv, SIGNAL(newConnection()), this, SLOT(newsession()));

where the slot looks like this":

void clienthandler::newsession() // Now i dont need to create a QTcpSocket to get the descriptor
{ // the nextConnection function of my QtcpServer class returns
QThread *t; // the descriptor directly
incomingslientsession *client;
while (serv->hasPendingConnections()){
t = new QThread();
client = new incomingslientsession(&db, serv->nextConnection(), &mutex);
connect(t, SIGNAL(started()), client, SLOT(dowork()));
connect(t, SIGNAL(finished()), t, SLOT(deleteLater())); // Delete thread after session is handeled
client->moveToThread(t);
t->start();
}
}

as you can see in the above code i dont have to work with any QTcpSocket to pass the socket to another thread.

in the thread i can now use the descriptor to open a QtcpSocket and read it like this:

void incomingslientsession::dowork()
{
qDebug("Session handler thread id: %d", (int) QThread::currentThreadId());

QTcpSocket sock;
sock.setSocketDescriptor(sockfd); // Create socket directly from desciptor, reading and writing stuff to socket works fine now

while (sock.bytesAvailable() < 20) { // First 20 header bytes
if (!sock.waitForReadyRead()) {
qDebug("Disconnected");
qDebug() << sock.error();
return;
}
}
// Do stuff with data here


this code works as it should and does not give me any debug messages about the sockets not being handeled in the right way. I'm quite interested in what you think i was doing wrong in the earlier code so if you could explain?

Thank you,

Sisco

[wysota]

I'm quite interested in what you think i was doing wrong in the earlier code so if you could explain?

Sure.

1. You were accessing the same socket from two threads
2. I'm suspecting you might be doing the same thing with the database which will cause you to obtain strange effects like losing rows in select results.
3. You are using threads where they are not needed (and there is no benefit for them to be there) which opens your program to DOS attacks - think what happens if one tries to make a really large number of concurrent connections to your server where each connection spawns a new thread (and the number of threads per process is usually limited) and waits unlimited time to receive 20 bytes of data (which will quickly exhaust either the limit of open descriptors, the limit of threads or just exhaust your ram).

Your whole code can be substituted by less than 20-30 lines of code doing all you require in a single thread without any pitfalls or problems.

[sisco]

1. You were accessing the same socket from two threads
Indeed, this was causing the problem with reading the socket.


2. I'm suspecting you might be doing the same thing with the database which will cause you to obtain strange effects like losing rows in select results.
If i want to access the database from a thread i do that using a new QSqlQuery object by using
mutex.lock();
QSqlQuery q(db);
mutex.unlock(); Wouldnt this protect that from happening?


3. You are using threads where they are not needed (and there is no benefit for them to be there) which opens your program to DOS attacks - think what happens if one tries to make a really large number of concurrent connections to your server where each connection spawns a new thread (and the number of threads per process is usually limited) and waits unlimited time to receive 20 bytes of data (which will quickly exhaust either the limit of open descriptors, the limit of threads or just exhaust your ram).
correct, as you can see in the newer code the code will now timeout after a set amount of time (default 30 seconds). Also i create a thread for each connection because the i'm sending large amounts of data in a session (the 20 bytes was just for testing). There probably is a better way to handle this by using the readyRead() signal for eacht socket.



Your whole code can be substituted by less than 20-30 lines of code doing all you require in a single thread without any pitfalls or problems.

Currently my code for reading all the data from a connection is about 20 lines.

Thank you for your help and explanation!

Sisco

[wysota]

If i want to access the database from a thread i do that using a new QSqlQuery object by using
mutex.lock();
QSqlQuery q(db);
mutex.unlock(); Wouldnt this protect that from happening?
No. You need a separate connection (aka QSqlDatabase instance).


correct, as you can see in the newer code the code will now timeout after a set amount of time (default 30 seconds).
I (as a DOS attacker) can be feeding 1 octet of data every 29 seconds in all 30000 (or more) connections I can open within a couple of seconds.


Also i create a thread for each connection because the i'm sending large amounts of data in a session (the 20 bytes was just for testing).
That's doesn't need threading since no NIC can accept an unlimited amount of data in a short period of time.


Currently my code for reading all the data from a connection is about 20 lines.
I mean the whole program :)

[sisco]

Okay, so this means im doing it all wrong :p

How would i do this without using a thread for each connection?
using the readyread() signal I would have to create a sepperate databuffer for each connection so that i can store the incoming data for the right client. Lets say 10 clients are finished sending all their data at around the same time and my software starts parsing their data one by one, if the server takes too long trying to parse the data, access the database and maybe send back some data (say 2 seconds per connection, or maybe 0,1 second per client with 100+ clients) of each client wouldnt this block the GUI? should i then start the parsing of data in another thread?

Sisco

[wysota]

using the readyread() signal I would have to create a sepperate databuffer for each connection so that i can store the incoming data for the right client.
You should do that regardless if you use readyRead() or not.


Lets say 10 clients are finished sending all their data at around the same time and my software starts parsing their data one by one, if the server takes too long trying to parse the data
You can do the parsing in worker threads. That's where they are becoming useful and that's where you can easily control the flow by allowing a defined maximum number of threads. Remaining requests will be queued until a thread is available (See QtConcurrent::run() and QThreadPool).


access the database and maybe send back some data (say 2 seconds per connection, or maybe 0,1 second per client with 100+ clients) of each client wouldnt this block the GUI?
It would if you were doing that in the GUI thread. However doing too many concurrent database connections would kill your database too so again thread-per-connection design is not a good approach. This is where a thread pool comes useful too.