PDA

View Full Version : QTcpSocket readyRead strange behavior



naroin
18th January 2011, 13:47
hi,

i've made a simple client/server example :

* The client connects to the server
* the server regulary sends data :
** size of buffer (short)
** buffer index (unsigned long)
** buffer, filled with zeros
* the client receives this data
** reads size
** reads buffer
** prints index.

Packet size should be always the same (19998).
But randomly, the client doesn't read the size, but reads 0. It seems there was a problem while reading the stream.

here's the log of my application :
...
onSocketReadyRead bytes= 7504
Bytes= 7504
Packet size= 19998
not enough data in stream
onSocketReadyRead bytes= 15008
Bytes= 15008
Packet size= 19998
not enough data in stream
onSocketReadyRead bytes= 20000
Bytes= 20000
Packet size= 19998
data read, packet index= 6733
onSocketReadyRead bytes= 0
onSocketReadyRead bytes= 3752
Bytes= 3752
Packet size= 0 <-- ERROR!

here's the code of the QThread client :


Client::Client(QObject *parent) : QThread(parent)
{

}

Client::~Client()
{

}

void Client::run()
{
pSocket = new QTcpSocket(this);

connect(pSocket, SIGNAL(readyRead()), this, SLOT(onSocketReadyRead()));

pSocket->connectToHost("192.168.50.77", 1234);
if (!pSocket->waitForConnected())
{
qDebug()<<"Unable to connect";
return;
}

exec();
}

void Client::onSocketReadyRead()
{
short sSize;
char* data;
unsigned long lPacketIndex;
qint64 nBytes = pSocket->bytesAvailable();

qDebug()<<"onSocketReadyRead bytes="<<nBytes;

if (nBytes < 2)
return;
qDebug()<<"Bytes="<<nBytes;
pSocket->peek((char*)&sSize, 2);

qDebug()<<"Packet size="<<sSize;
if (sSize != 19998)
{
QMessageBox::critical(NULL, tr("error"), tr("This is it"));
pSocket->disconnectFromHost();
return;
}

if (nBytes < 2 + sSize)
{
qDebug()<<" not enough data in stream";
return;
}

data = new char[sSize];

pSocket->read((char*)&sSize, 2);
pSocket->read(data, sSize);

memcpy((char*)&lPacketIndex, data, 4);

qDebug()<<" data read, packet index="<<lPacketIndex;

delete data;
}


What do i do that i should not do?
Is it a Qt bug?
This problem is random but can be reproduced more easily with a corrupted network (bad wifi connection, etc...)
it MAY be caused by TCP packet retransmission on timeout (i've seen such retransmission with wireshark)

thanks

high_flyer
18th January 2011, 14:05
Packet size should be always the same (19998).
I don't think you have control over the size of the underlying packet. (the ones the server or any servers in between are packing).
What you can do is wait in onSocketReadRead() until the at least 19998 bytes are available before you read.

wysota
18th January 2011, 14:12
What do i do that i should not do?
You assume that if some data arrives then all data arrives which is not the case. TCP is a stream of bytes without any internal structure. The fact that the sender sends 5 different chunks of data doesn't mean it will also arrive in 5 chunks. It may be one chunk or 99999+ chunks.

naroin
18th January 2011, 14:13
thats what i do.

i ready 2 bytes, that should contain the packet size (here always 19998), then i read the buffer.
As a consequence, i should always read 19998 as the size, but i dont.

wysota
18th January 2011, 14:16
i ready 2 bytes, that should contain the packet size (here always 19998), then i read the buffer.
"packet" is the term reserved for Internel Protocol (IP). TCP works in a concept of "segments". So 19998 is definitely not the packet size. You are sending a stream of 19998 bytes, that's it. You never know across how many packets or segments the data spans.


As a consequence, i should always read 19998 as the size, but i dont.
No, you shouldn't. Eventually you will get 20000 (2+19998) bytes at the receiving side but there is no guarantee they will arrive at the same time.

naroin
18th January 2011, 14:21
ok, sorry, my terms are not very clear :)
19998 is my DATA size.
i'm sending a stream of 20000 bytes, the first 2 containing the size of the rest of the data (that is, 19998).

i know my data won't arrive at the same time, and if you look at the code, i verify that i have enough data before reading it.
If i do it wrong, i really don't know how i can do right.

What should i change in my code to make it works?

high_flyer
18th January 2011, 14:30
What should i change in my code to make it works?
Do what I answered you in my previous post!

wysota
18th January 2011, 14:32
Well, I would definitely not cast char* to short like that. It is possible the sending side doesn't send it the way you expect it and you get bogus data.

Try something like:

QByteArray buffer; // somewhere
void X::onReadyRead() {
buffer += socket->readAll(); // read all pending data

while(buffer.size()>=2){
quint16 size = (buffer.at(0) << 8) + buffer.at(1); // 16bit unsigned value
if(buffer.size() <= 2+size) return;
QByteArray data = buffer.mid(2, size);
processData(data);
buffer.remove(0,2+size);
}
}

Of course adjust the sending side to send the size in the same manner.

naroin
18th January 2011, 15:14
high_flyer i really dont understand, i'm sorry if i am stupid, i DONT ASSUME that all data arrives if SOME data arrives.
Check the code:


qint64 nBytes = pSocket->bytesAvailable();

if (nBytes < 2)
return;
pSocket->peek((char*)&sSize, 2);

if (nBytes < 2 + sSize)
{
qDebug()<<" not enough data in stream";
return;
}


Added after 32 minutes:

wysota, i've got exactly the same problem.

Sometimes, i read a size of 0 instead of 19998.
i've changed the content of my data
memset(data, 1, 19998);
then the size i read is 257. (0x0101)

as a consequence, there seems to be a shift in stream. Can it be due to TCP retransmission on error?
Windows socket bug? Qt bug?

thanks for your help.

naroin
19th January 2011, 09:14
no more help?

high_flyer
19th January 2011, 09:32
I don't understand what help you want.
Can you show your current code?
Do you read until you get the buffer in the size you expect?

naroin
19th January 2011, 09:35
yes i read until i get the buffer in the size i expect

the code is in my first post... where is it false??

void Client::onSocketReadyRead()

high_flyer
19th January 2011, 10:00
Your code in the first post does NOT wait for the expected size:


qint64 nBytes = pSocket->bytesAvailable(); //you check bytes available

qDebug()<<"onSocketReadyRead bytes="<<nBytes;

if (nBytes < 2) // it could be you get 0 or 1 bytes, so you get out here. But lets say you got 3, so we go on.
return;
qDebug()<<"Bytes="<<nBytes;
pSocket->peek((char*)&sSize, 2);//why do you use peek and not bytesAvailable? also, why do you peel only 2 bytes ahead? it will alway be at most 2 bytes this the following if will always get out.

qDebug()<<"Packet size="<<sSize;

if (sSize != 19998) //so we didn't read 19998, (sSize will be at most 2 because of the peek) which is the size you are waiting for, so we get in to the if.
{
QMessageBox::critical(NULL, tr("error"), tr("This is it"));
pSocket->disconnectFromHost();
return; //And what do you do? you get out!!
}

if (nBytes < 2 + sSize)
{
qDebug()<<" not enough data in stream";
return;
}



EDIT:

if (sSize != 19998)
is a tottaly a problem.
Read the peek documentation and be sure to understand it.
sSize should be a buffer, as long as the the data you want to read.
It does not store the size read size, but the read data!
If at all, you probably then use it like this:


int iSize = peel(buff,maySize);

if(iSize != iExpectedSize){...}

naroin
19th January 2011, 10:43
the buffer i want to stream contains:
2 bytes, containing the size of the buffer
rest of the buffer.
in the final program, the size may differ from one packet to another, so i have to put it at the beginning of the send data.
For my tests here, i put it at 19998.

the goal is to read
* the size (in the 2 first bytes)
* then the buffer, corresponding to size just read
then redo the same.

here's my code with comments:



void Client::onSocketReadyRead()
{
short sSize;
char* data;
unsigned long lPacketIndex;
qint64 nBytes = pSocket->bytesAvailable();

qDebug()<<"onSocketReadyRead bytes="<<nBytes;

if (nBytes < 2) //we haven't read enough data to get the buffer size, return
return;
qDebug()<<"Bytes="<<nBytes;
pSocket->peek((char*)&sSize, 2); //we read 2 bytes, we get the size of the buffer INSIDE these 2 bytes
//here i use peek to get the 2 bytes containing the size, without removing it from the buffer, because i may have not enough data, and i'll have to redo it.

qDebug()<<"Packet size="<<sSize;
/* this IF is only to get easily the error . It's DEBUG code.
as i KNOW the size must be 19998, if it's not, there was an error.
*/
if (sSize != 19998)
{
//i put a breakpoint here, the code stops.
QMessageBox::critical(NULL, tr("error"), tr("This is it"));
pSocket->disconnectFromHost();
return;
}

if (nBytes < 2 + sSize) //here we wait to have enough data to read sSize bytes
{
qDebug()<<" not enough data in stream";
return;
}

data = new char[sSize];

pSocket->read((char*)&sSize, 2);
pSocket->read(data, sSize);

memcpy((char*)&lPacketIndex, data, 4);

qDebug()<<" data read, packet index="<<lPacketIndex;

delete data;
}


is it clearer?

high_flyer
19th January 2011, 10:48
wysota gave you the solution, why don't you use it?

naroin
19th January 2011, 10:57
I have answrered : i've exactly the same problem with wysota's solution.

As i already said, it works well for a certain time, then it randomly fail. In addition, it does not fail in a "good" network with no TCP retransmission, and often fails in a "bad" network, like low wifi.

well, i'll use directly windows sockets.

wysota
19th January 2011, 11:33
Can we see the code for the sender? Also are the sender and the receiver running the same operating system and the same CPU type?

naroin
19th January 2011, 11:44
yes, they are both on windows,

here's the server side code :


#define DATA_SIZE 20000

Server::Server(QObject *parent) : QThread(parent)
{

}

Server::~Server()
{

}

void Server::run()
{
pServer = new QTcpServer();
pMapperReadyRead = new QSignalMapper();
pMapperDisconnected = new QSignalMapper();
pMapperError = new QSignalMapper();

connect(pServer, SIGNAL(newConnection()), this, SLOT(onNewConnection()), Qt::DirectConnection);
connect(pMapperReadyRead, SIGNAL(mapped(QObject*)), this, SLOT(onSocketReadyRead(QObject*)), Qt::DirectConnection);
connect(pMapperDisconnected, SIGNAL(mapped(QObject*)), this, SLOT(onSocketDisconnected(QObject*)), Qt::DirectConnection);

pServer->listen(QHostAddress::Any, 1234);

//timer d'envoi
lPacketIndex = 0;
QTimer* pTimer = new QTimer(this);
connect(pTimer, SIGNAL(timeout()), this, SLOT(onTimeout()));

pTimer->setSingleShot(false);
pTimer->start(100);



exec();

pServer->close();
delete pServer;

delete pMapperReadyRead;
delete pMapperDisconnected;
delete pMapperError;
}

void Server::onNewConnection()
{
QTcpSocket* pSocket = pServer->nextPendingConnection();
if (pSocket == NULL)
return;

qDebug()<<"new connection";

vecClient.push_back(pSocket);

connect(pSocket, SIGNAL(readyRead()), pMapperReadyRead, SLOT(map()), Qt::DirectConnection);
pMapperReadyRead->setMapping(pSocket, pSocket);
connect(pSocket, SIGNAL(disconnected()), pMapperDisconnected, SLOT(map()), Qt::DirectConnection);
pMapperDisconnected->setMapping(pSocket, pSocket);
}

void Server::onSocketReadyRead( QObject* pObject )
{

}

void Server::onSocketDisconnected( QObject* pObject )
{
int i;
QTcpSocket* pSocket = (QTcpSocket*)pObject;

qDebug()<<"Socket disconnected";

for (i=0;i<vecClient.size();++i)
{
if (pSocket = vecClient[i])
{
vecClient.erase(vecClient.begin() + i);
break;
}
}

pSocket->deleteLater();
}

void Server::onSocketError( QObject* pObject, QAbstractSocket::SocketError error )
{
QTcpSocket* pSocket = (QTcpSocket*)pObject;

qDebug()<<"Socket error : "<<pSocket->errorString();
}

void Server::onTimeout()
{
int i;
char data[DATA_SIZE];
short sSize = DATA_SIZE - 2;


memset(data, 0, DATA_SIZE);
memcpy(data, &sSize, 2);
memcpy(data+2, &lPacketIndex, 4);

++lPacketIndex;

for (i=0;i<vecClient.size();++i)
{
vecClient[i]->write(data, DATA_SIZE);

}
}


i can also send you VC++ projects, if you need them

wysota
19th January 2011, 11:47
This code is not compliant with what I have shown you. You have not implemented what I suggested so do that first and then we can continue.

naroin
19th January 2011, 11:50
yeah, that's my server.

the client connects to it, then the server regulary sends data (onTimeout)
In my project, it's the server that pushes bytes in the socket.

as you can see line 108 :

vecClient[i]->write(data, DATA_SIZE);

wysota
19th January 2011, 11:51
You are sending raw bytes and interpreting them on the other end as integers. Don't do that. I told you to encode the size explicitly. If your sender is a 32 bit OS and your receiver a 64 bit OS (or the other way round) your code will fail immediately.

naroin
19th January 2011, 11:56
you're totally right, but for this example, that's not the point : the code does not fail immediately.
As i am on 2 32bits windows, it works.

(does the size of "short" change between 32 and 64bit OS? i though it could be a problem only if the endianness changed)

wysota
19th January 2011, 12:25
It is the point because it is this exact variable that is giving you an incorrect result. So first make sure the problem is not there (and not assume it is not there) and then continue elsewhere.

This is sender side compatible with my previous suggestion.


QByteArray data; // external byte array containing your data to be sent

quint16 s = data.size();
QByteArray ba(2, 0);
ba[0] = (s >> 8) & 0xFF;
ba[1] = s & 0xFF;
ba.append(data);
for (i=0;i<vecClient.size();++i)
{
vecClient[i]->write(ba);
}

naroin
19th January 2011, 13:28
i've got the same problem.

receiver side code :


void Client::onSocketReadyRead()
{
bufferSocket += pSocket->readAll();

while (bufferSocket.size() >= 2)
{
quint16 size = (bufferSocket.at(0) << 8) + bufferSocket.at(1); // 16bit unsigned value

qDebug()<<"size="<<size;

if (size != 19998)
{

QMessageBox::critical(NULL, tr("error"), tr("This is it"));
pSocket->disconnectFromHost();
return;
}

if(bufferSocket.size() <= 2+size) return;

qDebug()<<"data read";

QByteArray data = bufferSocket.mid(2, size);

bufferSocket.remove(0,2+size);
}


sender side code:


void Server::onTimeout()
{
QByteArray data;
int i;

data.fill(1, 19998);

quint16 s = data.size();
QByteArray ba(2, 0);
ba[0] = (s >> 8) & 0xFF;
ba[1] = s & 0xFF;
ba.append(data);
for (i=0;i<vecClient.size();++i)
{
vecClient[i]->write(ba);
}
}

wysota
19th January 2011, 13:37
Dump the data you receive on the receiving end to see whether you are not losing synchronization with your stream.

By the way, if your data is of constant size then why are you sending the size with each bucket?

naroin
19th January 2011, 13:52
the data is of constant size just for these tests. It will change in the real project :)

I think indeed i've a synchronisation problem. How can it be, as i am in TCP mode?
what do you mean by dump?
should i make a readAll after receiving a full bucket?

wysota
19th January 2011, 15:10
I think indeed i've a synchronisation problem. How can it be, as i am in TCP mode?
If you read less data than you write then you lose synchronisation.


what do you mean by dump?
Display on console.

naroin
19th January 2011, 16:23
that is strange.

logging code:


QString strDebug;
quint8 c, c2;
int nCount = 0;
c2 = data.at(0);
for (i=1;i<data.size();++i)
{
++nCount;
c = data.at(i);
if (c != c2)
{
strDebug += tr("%1 * %2 | ").arg(c2).arg(nCount);

c2 = c;
nCount = 0;
}

}
++nCount;
strDebug += tr("%1 * %2 | ").arg(c2).arg(nCount);
qDebug()<<strDebug;


logs:
...
size= 19998
data read
"1 * 19998 | "
size= 19998
size= 19998
size= 19998
data read
"1 * 12494 | 78 * 1 | 30 * 1 | 1 * 7502 | "



i should have only "1" in my data, but i dont have.
78/30 is 4E1E = 19998, that's my size.
it is as if some data were dropped after my 12494 bytes

the server side sends correct data (1* 19998).

wysota
19th January 2011, 16:41
the server side sends correct data (1* 19998).
How do you know that? Do you check the return value of QIODevice::write()? Do you also check the how many bytes are really read on the receiving end?

By the way, get rid of threads, you don't need them and they might be influencing your results somehow.

naroin
19th January 2011, 17:01
i just changed the receiver side code with windows sockets : it works perfectly.
So
* that's not a sender side problem
* that's not a thread problem
* i think there may be a bug in QTcpSocket::readyRead, but that's very strange as it seems i'm the only guy in the world to have it.

thanks for your help

wysota
19th January 2011, 17:45
The bug is most likely in the way you use the library. You couldn't have modified your code "in-place", you must have rewritten many aspects of your program changing the way your application works. Somehow strangely thousands of applications work with QTcpSocket without problems and suddenly you believe that there is a bug there because in your application something doesn't work. Think yourself what is more probable.

naroin
20th January 2011, 09:07
yes that's what i think, but noone here seems to be able to say me what is wrong !

i used the code you sent me, and i got the same problem.
My test applications don't do anything than launching the threads whom i've sent you the code. I really dont understand what can go wrong!

what do you want i send you so you can be able to say me what i did wrong? i really want to know, and you're right, i think it's my fault, but i dont know why.

maybe a problem in the initialization ?


void Client::run()
{
pSocket = new QTcpSocket(this);

connect(pSocket, SIGNAL(readyRead()), this, SLOT(onSocketReadyRead()));

pSocket->connectToHost("192.168.50.77", 1234);
if (!pSocket->waitForConnected())
{
qDebug()<<"Unable to connect";
return;
}

exec();
}

tbscope
20th January 2011, 10:09
Note that the slot you call from the thread is not performed inside the thread.

naroin
20th January 2011, 10:46
how can i do so it is in the thread?
This is one of the things i don't understand well about Qt & threads.

Added after 26 minutes:

i've created a new class ClientInternal, and in my Client code:


void Client::run()
{
ClientInternal ci;

exec();
}

my ClientInternal code:

ClientInternal::ClientInternal(QObject *parent)
: QObject(parent)
{
pSocket = new QTcpSocket(this);

connect(pSocket, SIGNAL(readyRead()), this, SLOT(onSocketReadyRead()));

pSocket->connectToHost("192.168.50.77", 1234);
if (!pSocket->waitForConnected())
{
qDebug()<<"Unable to connect";
return;
}
}

now it seems to work!
why?

wysota
20th January 2011, 11:57
Because you have the event loop running. Anyway, get rid of the threads, you really don't need them since socket communication is asynchronous.

naroin
20th January 2011, 13:28
okay but why some data is dropped with my "false thread" ?
is there a event loop for each thread of one for all the application?

squidge
20th January 2011, 14:17
Each thread can have its own event loop, but it's not automatic, you have to execute it yourself.

wysota
20th January 2011, 14:23
okay but why some data is dropped with my "false thread" ?
is there a event loop for each thread of one for all the application?

Remove threads and all should be working fine. We must have missed the fact you were calling a slot of the QThread object.

naroin
20th January 2011, 14:59
yes it works well without my QThread object.
But can i have a technical explanation of WHY it didn't work well?
I think i'm missing something important here, and i would like to understand.

However, thanks for your help so far

wysota
20th January 2011, 15:28
But can i have a technical explanation of WHY it didn't work well?
Sure. Your QThread object "lives" in the main thread which means all events for it are processed by the main thread. Your socket lives in the thread controlled by the QThread object. This means the socket gets notified about incoming data by the worker thread and you try to read data from the main thread. Since there is no synchronization between threads here, data may be lost (if you read something and at the same time the other thread writes something to the socket object).

naroin
20th January 2011, 16:37
what you are saying is that i must not make calls to the QTcpSocket object's methods (that works in the QThread thread) with my main thread.

Well, that's logical.

thanks

tbscope
20th January 2011, 16:57
No, the problem is that part of the code that you think that runs in the thread does in fact not run in the thread.

All the code in the slot uses the main event loop while all the code in the run() function uses the thread event loop.

There are two ways to solve this:
1. As suggested, do not use threads.
2. Use threads correctly.

The last part is, for example, done by putting all your socket handling code in a separate QObject subclass.
Then move the object to a new thread. This way you're sure that the code in the slot also uses the thread event loop (because the complete object is moved to the thread).

marcvanriet
20th January 2011, 20:32
Hi,
You could look at the fortunecookie server and client examples.

In that example the length of the "packet" is put upfront. On the receiving side, the length is read, and they keep on receiving until all the bytes are received. This is what you want.

It is best to see a TCP connection as a 'stream' of bytes that are coming in. You have to find the start and end of your 'message' yourself.

Best regards,
Marc

wysota
20th January 2011, 20:57
In that example the length of the "packet" is put upfront. On the receiving side, the length is read, and they keep on receiving until all the bytes are received. This is what you want.
That's exactly what he has, you know ;)

marcvanriet
20th January 2011, 21:06
Oops... didn't go to the very last page of this thread. Well, better safe then sorry...

naroin
21st January 2011, 15:09
yeah, i think i'll just use threads correctly. I didn't understand well.

wysota
21st January 2011, 23:28
yeah, i think i'll just use threads correctly. I didn't understand well.

The point we are trying to stress is that using threads doesn't give you any benefit over not using threads and at the same time it gives you many problems (you have already stumbled upon one of them).