Bytes are lost when sending data from C socket to QTcpSocket

[craigt]

Hi.

I'm developing a client/server architecture which opens files on a client machine, sends them to a server which does some number crunching and returns a new data type back, 1 for each file received, which is then written to file by the client. I've been struggling with bytes disappearing on the return connection (separate socket to the sending connection).

All components are currently on a Linux x86_64 platform.

I'm not an expert on network programming by any length so I've revise this design many times but I've been unable to iron out the problem. The data structure being sent back (called ARD) is similar to a BMP image. It consists of a fixed size header which contains a bunch of info about the data including dimensions of a matrix of 32 bit floats. The matrix is then follow by a string which is for general commenting purposes. The offset and length of the string are stored in the header.

The socket on the server side is a normal C type socket (sys/socket.h). The server is not a Qt app, its normal C++ command line. The client side is a Qt GUI app so I'm using QTcpSocket which belongs to a non-GUI thread.

Currently the behavior is quite random. I can successfully get and random number of files back, up to a maximum of about 30 consecutively when things are going well. But at some point I will see corruption of the incoming header and the process comes to a holt as I need to dynamically assign space for the matrix which will now be some random number. This suggests to me that bytes are going missing in the stream which shouldn't really happen in a properly implemented TCP connection.

The sending function of the server is as follows:

int& operator<<(int& sock, Ard& ard)
{
    //Set socket fd to blocking mode (Sockets from Qt API are non-blocking by default)
    int i = fcntl(sock, F_GETFL);
    i = ( i & ~O_NONBLOCK );
    fcntl ( sock, F_SETFL,i );
 
    ard.updateSizes(); // corrects comment length and offset as well as file size variables
 
    unsigned int writeTotal = 0;
    char header[Ard::HEADER_SIZE];
 
    int bytesWritten;
 
    //serialise header
    memcpy(header, ard.m_fileType, 4);
    memcpy(header + 4, &ard.m_ardType, 1);
    memcpy(header + 5, &ard.m_zTime, 8);
    memcpy(header + 13, &ard.m_fc, 4);
    memcpy(header + 17, &ard.m_fs, 4);
    memcpy(header + 21, &ard.m_bw, 4);
    memcpy(header + 25, &ard.m_minAmp, 4);
    memcpy(header + 29, &ard.m_maxAmp, 4);
    memcpy(header + 33, &ard.m_rangeRes, 4);
    memcpy(header + 37, &ard.m_dopplerRes, 4);
    memcpy(header + 41, &ard.m_xDim, 4);
    memcpy(header + 45, &ard.m_yDim, 4);
    memcpy(header + 49, &ard.m_txRxDist, 4);
    memcpy(header + 53, &ard.m_cOffset, 8);
    memcpy(header + 61, &ard.m_cLength, 4);
    memcpy(header + 65, &ard.m_fileSize, 8);
 
    char *ptr = header;
    int bytesLeft = Ard::HEADER_SIZE;
 
    while (bytesLeft)
    {
        bytesWritten = send(sock, ptr, bytesLeft, 0);
        ptr += bytesWritten;
        writeTotal += bytesWritten;
        bytesLeft -= bytesWritten;
 
        if(bytesWritten == -1)
            cout << "Ard::operator<<(): Error: can't write to socket' " << endl;
    }
 
    cout << "Ard::operator<<(): After header writeTotal = " << writeTotal << endl;
 
    for(unsigned int x = 0; x < ard.m_xDim; x++)
    {
        bytesLeft = ard.m_yDim * sizeof(float);
        ptr = (char*)ard.m_data[x];
        while(bytesLeft)
        {
            bytesWritten = send (sock, ptr, bytesLeft, 0);
            bytesLeft -= bytesWritten;
            writeTotal += bytesWritten;
            ptr += bytesWritten;
 
            if(bytesWritten == -1)
                cout << "Ard::operator<<(): Error: can't write to socket' " << endl;
        }
    }
 
    cout << "Ard::operator<<(): After data block writeTotal = " << writeTotal << endl;
 
    bytesLeft = ard.m_cLength;
    char comment[ard.m_cLength];
    strncpy (comment, ard.m_comment.c_str(), ard.m_cLength);
    ptr = comment;
 
    while(bytesLeft)
    {
        bytesWritten = send (sock, ptr, bytesLeft, 0);
        bytesLeft -= bytesWritten;
        writeTotal += bytesWritten;
        ptr += bytesWritten;
 
        if(bytesWritten == -1)
            cout << "Ard::operator<<(): Error: can't write to socket' " << endl;
    }
 
    cout << "Ard::operator<<(): At end of ARD writing writeTotal = " << writeTotal << endl;
    return sock;
}

int& operator<<(int& sock, Ard& ard)
{
    //Set socket fd to blocking mode (Sockets from Qt API are non-blocking by default)
    int i = fcntl(sock, F_GETFL);
    i = ( i & ~O_NONBLOCK );
    fcntl ( sock, F_SETFL,i );

    ard.updateSizes(); // corrects comment length and offset as well as file size variables

    unsigned int writeTotal = 0;
    char header[Ard::HEADER_SIZE];

    int bytesWritten;

    //serialise header
    memcpy(header, ard.m_fileType, 4);
    memcpy(header + 4, &ard.m_ardType, 1);
    memcpy(header + 5, &ard.m_zTime, 8);
    memcpy(header + 13, &ard.m_fc, 4);
    memcpy(header + 17, &ard.m_fs, 4);
    memcpy(header + 21, &ard.m_bw, 4);
    memcpy(header + 25, &ard.m_minAmp, 4);
    memcpy(header + 29, &ard.m_maxAmp, 4);
    memcpy(header + 33, &ard.m_rangeRes, 4);
    memcpy(header + 37, &ard.m_dopplerRes, 4);
    memcpy(header + 41, &ard.m_xDim, 4);
    memcpy(header + 45, &ard.m_yDim, 4);
    memcpy(header + 49, &ard.m_txRxDist, 4);
    memcpy(header + 53, &ard.m_cOffset, 8);
    memcpy(header + 61, &ard.m_cLength, 4);
    memcpy(header + 65, &ard.m_fileSize, 8);

    char *ptr = header;
    int bytesLeft = Ard::HEADER_SIZE;

    while (bytesLeft)
    {
        bytesWritten = send(sock, ptr, bytesLeft, 0);
        ptr += bytesWritten;
        writeTotal += bytesWritten;
        bytesLeft -= bytesWritten;

        if(bytesWritten == -1)
            cout << "Ard::operator<<(): Error: can't write to socket' " << endl;
    }

    cout << "Ard::operator<<(): After header writeTotal = " << writeTotal << endl;

    for(unsigned int x = 0; x < ard.m_xDim; x++)
    {
        bytesLeft = ard.m_yDim * sizeof(float);
        ptr = (char*)ard.m_data[x];
        while(bytesLeft)
        {
            bytesWritten = send (sock, ptr, bytesLeft, 0);
            bytesLeft -= bytesWritten;
            writeTotal += bytesWritten;
            ptr += bytesWritten;

            if(bytesWritten == -1)
                cout << "Ard::operator<<(): Error: can't write to socket' " << endl;
        }
    }

    cout << "Ard::operator<<(): After data block writeTotal = " << writeTotal << endl;

    bytesLeft = ard.m_cLength;
    char comment[ard.m_cLength];
    strncpy (comment, ard.m_comment.c_str(), ard.m_cLength);
    ptr = comment;

    while(bytesLeft)
    {
        bytesWritten = send (sock, ptr, bytesLeft, 0);
        bytesLeft -= bytesWritten;
        writeTotal += bytesWritten;
        ptr += bytesWritten;

        if(bytesWritten == -1)
            cout << "Ard::operator<<(): Error: can't write to socket' " << endl;
    }

    cout << "Ard::operator<<(): At end of ARD writing writeTotal = " << writeTotal << endl;
    return sock;
}

To copy to clipboard, switch view to plain text mode 

and the receive code on the client side

void QArd::readData(QTcpSocket& sock)
{
    int bytesRead;
    int bytesLeft;
    char* ptr;
 
    if(!m_headerRead)
    {
        if(sock.bytesAvailable() < HEADER_SIZE) //wait for all header data to be available
        {
            qDebug() << "----socket receving function returning at header";
            return;
        }
 
        bytesLeft = HEADER_SIZE;
        char header[HEADER_SIZE];
        ptr = header;
 
        while (bytesLeft)
        {
            bytesRead = sock.read(ptr, bytesLeft);
            ptr += bytesRead;
            m_readTotal += bytesRead;
            bytesLeft -= bytesRead;
        }
 
        //deserialise header
        memcpy(m_fileType, header, 4);
        memcpy(&m_ardType, header + 4, 1);
        memcpy(&m_zTime, header + 5, 8);
        memcpy(&m_fc, header + 13, 4);
        memcpy(&m_fs, header + 17, 4);
        memcpy(&m_bw, header + 21, 4);
        memcpy(&m_minAmp, header + 25, 4);
        memcpy(&m_maxAmp, header + 29, 4);
        memcpy(&m_rangeRes, header + 33, 4);
        memcpy(&m_dopplerRes, header + 37, 4);
        memcpy(&m_xDim, header + 41, 4);
        memcpy(&m_yDim, header + 45, 4);
        memcpy(&m_txRxDist, header + 49, 4);
        memcpy(&m_cOffset, header + 53, 8);
        memcpy(&m_cLength, header + 61, 4);
        memcpy(&m_fileSize, header + 65, 8);
 
        qDebug() << getInfoString().c_str();
 
        qDebug() << "QArd:: read After header readTotal = " << m_readTotal;
 
        allocateMatrix();
 
 
        m_headerRead = true;
    }
 
 
 
    while(m_rangeBin < m_xDim)
    {
        if((unsigned int)sock.bytesAvailable() < sizeof(float) * m_yDim)
        {
            qDebug() << "----socket receving function returning at data block";
            return;
        }
 
        bytesLeft = m_yDim * sizeof(float);
        ptr = (char*)m_data[m_rangeBin];
 
        while(bytesLeft)
        {
            bytesRead = sock.read(ptr, bytesLeft);
            m_readTotal += bytesRead;
            ptr += bytesRead;
            bytesLeft -= bytesRead;
        }
 
        m_rangeBin++;
    }
 
    qDebug() << "QArd:: readData() After data block readTotal = " << m_readTotal;
 
    if(sock.bytesAvailable() < m_cLength)
    {
        qDebug() << "----socket receving function returning at comment";
        return;
    }
 
    char comment[m_cLength];
    ptr = comment;
    bytesLeft = m_cLength;
 
    while(bytesLeft)
    {
        bytesRead = sock.read(ptr, bytesLeft);
        m_readTotal += bytesRead;
        ptr += bytesRead;
        bytesLeft -= bytesRead;
    }
 
    qDebug() << "COMMENT = " << comment;
 
    m_comment = comment;
 
 
    qDebug() << m_comment.c_str();
    qDebug() << "QArd:: readData() After ARD read finished readTotal = " << m_readTotal;
 
    //reset parameters to start reading new plot:
    m_rangeBin = 0;
    m_headerRead = false;
    m_readTotal = 0;
 
    //send signal to say reading is complete
    readingComplete();
}

void QArd::readData(QTcpSocket& sock)
{
    int bytesRead;
    int bytesLeft;
    char* ptr;

    if(!m_headerRead)
    {
        if(sock.bytesAvailable() < HEADER_SIZE) //wait for all header data to be available
        {
            qDebug() << "----socket receving function returning at header";
            return;
        }

        bytesLeft = HEADER_SIZE;
        char header[HEADER_SIZE];
        ptr = header;

        while (bytesLeft)
        {
            bytesRead = sock.read(ptr, bytesLeft);
            ptr += bytesRead;
            m_readTotal += bytesRead;
            bytesLeft -= bytesRead;
        }

        //deserialise header
        memcpy(m_fileType, header, 4);
        memcpy(&m_ardType, header + 4, 1);
        memcpy(&m_zTime, header + 5, 8);
        memcpy(&m_fc, header + 13, 4);
        memcpy(&m_fs, header + 17, 4);
        memcpy(&m_bw, header + 21, 4);
        memcpy(&m_minAmp, header + 25, 4);
        memcpy(&m_maxAmp, header + 29, 4);
        memcpy(&m_rangeRes, header + 33, 4);
        memcpy(&m_dopplerRes, header + 37, 4);
        memcpy(&m_xDim, header + 41, 4);
        memcpy(&m_yDim, header + 45, 4);
        memcpy(&m_txRxDist, header + 49, 4);
        memcpy(&m_cOffset, header + 53, 8);
        memcpy(&m_cLength, header + 61, 4);
        memcpy(&m_fileSize, header + 65, 8);

        qDebug() << getInfoString().c_str();

        qDebug() << "QArd:: read After header readTotal = " << m_readTotal;

        allocateMatrix();


        m_headerRead = true;
    }



    while(m_rangeBin < m_xDim)
    {
        if((unsigned int)sock.bytesAvailable() < sizeof(float) * m_yDim)
        {
            qDebug() << "----socket receving function returning at data block";
            return;
        }

        bytesLeft = m_yDim * sizeof(float);
        ptr = (char*)m_data[m_rangeBin];

        while(bytesLeft)
        {
            bytesRead = sock.read(ptr, bytesLeft);
            m_readTotal += bytesRead;
            ptr += bytesRead;
            bytesLeft -= bytesRead;
        }

        m_rangeBin++;
    }

    qDebug() << "QArd:: readData() After data block readTotal = " << m_readTotal;

    if(sock.bytesAvailable() < m_cLength)
    {
        qDebug() << "----socket receving function returning at comment";
        return;
    }

    char comment[m_cLength];
    ptr = comment;
    bytesLeft = m_cLength;

    while(bytesLeft)
    {
        bytesRead = sock.read(ptr, bytesLeft);
        m_readTotal += bytesRead;
        ptr += bytesRead;
        bytesLeft -= bytesRead;
    }

    qDebug() << "COMMENT = " << comment;

    m_comment = comment;


    qDebug() << m_comment.c_str();
    qDebug() << "QArd:: readData() After ARD read finished readTotal = " << m_readTotal;

    //reset parameters to start reading new plot:
    m_rangeBin = 0;
    m_headerRead = false;
    m_readTotal = 0;

    //send signal to say reading is complete
    readingComplete();
}

To copy to clipboard, switch view to plain text mode 

This function is called from another function which is connected to the readyRead() of the socket.

There is no extra control protocol between sends (which may be bad?) But my understanding is that it shouldn't be necessary except for some sort of network failure. That's not critical for the use case of the system though. The client should receive a known size header then from the header wait for a specified number of bytes of data and then a specified number of bytes for the comment string then wait for the next header. This way it will be easy to allow multiple clients to connect to the system at any point and start receiving the ARD data as well which out a complex sync with each client.

If anyone can offer any comments suggestions or point out bad design it would be great appreciated, I'm tearing my hair out over this. Also if there could be potential problems elsewhere I'll put up more of the program(s) but I've omitted them for now, they're quite big.

I am saving the ARD file to disk from the server just before its sent back to the client and I can confirm that the data is correct at that point. I do know that strictly the data should be converted to "network endianess" but thats more a of a cross platform problem. I do most of the testing on between client and server on the same PC. I've also tested it extensively across a network with the same results.

Kind regards.

[Peiyuan Li]

Sorry to interrupt you,I am a newcomer in here.I want to ask a question,but I don't know how to do it,could you tell me?Thank you very much!!!

[craigt]

Click forum in the top bar. Select the category your question applies to. (Probably Qt programming) Then click new thread and fill out the section similar to the way you replied to this thread.

[wysota]

First of all check that your socket is created in a proper thread. Then I suggest to pass sockets as pointers and not references. Then I suggest to not copy data you read directly into structures because of byte order issues. And finally I suggest to simplify your spaghetti code into several functions. Then if the problem persists, it should be easier to track it.

[craigt]

Ok thanks.

When you not don't copy data directly into structures should it go to a QByteArray first then and shift out of it?

Then regarding the thread.

I have inherited from QThread:

#include "ardThread.h"
#include <QDebug>
 
ArdThread::ArdThread(QObject *parent) : QThread(parent)
{
}
 
ArdThread::~ArdThread()
{
    qDebug() << "ARD Thread destructor (" << thread() << ")";
    wait();
}
 
void ArdThread::run()
{
    qDebug() << "ARD Thread started (" << thread() << ")";
 
    exec();
}

#include "ardThread.h"
#include <QDebug>

ArdThread::ArdThread(QObject *parent) : QThread(parent)
{
}

ArdThread::~ArdThread()
{
    qDebug() << "ARD Thread destructor (" << thread() << ")";
    wait();
}

void ArdThread::run()
{
    qDebug() << "ARD Thread started (" << thread() << ")";

    exec();
}

To copy to clipboard, switch view to plain text mode 

Then I have a class ArdHandler which has the QTcpSocket as a member (which is connected to the server from ArdHandler's constructor).
ArdHandler has a readyRead() connected to the member socket's readyRead() (connect also made in the constructor).
The QArd::readData() I posted previously is then called from ArdHandler::readReady()/

Then in MainWindow:

m_ardThread = new ArdThread();
        m_ardHandler = new ArdHandler();
 
       //connect signals/slots MainWindow to ArdHandler
 
        m_ardHandler->moveToThread(m_ardThread);
        m_ardThread->start();

m_ardThread = new ArdThread();
        m_ardHandler = new ArdHandler();

       //connect signals/slots MainWindow to ArdHandler

        m_ardHandler->moveToThread(m_ardThread);
        m_ardThread->start();

To copy to clipboard, switch view to plain text mode 

[wysota]

When you not don't copy data directly into structures should it go to a QByteArray first then and shift out of it?

You can do whatever you want as long as you make sure that a little-endian machine can read data sent by a big-endian machine and vice versa. And that different data alignment/structure packing on different platforms doesn't influence the data sent over the wire.

Then I have a class ArdHandler which has the QTcpSocket as a member (which is connected to the server from ArdHandler's constructor).

If ArdHandler is not set explicitly as a parent of the socket then your code is incorrect, the socket will remain in the thread where you constructed ArdHandler and you will be losing data from the socket. Just be aware that if you do set it as a parent, you will have to properly delete the object, otherwise you'll have double deletion problems. I think it is safer to not make the socket a member variable of the handler. If you want, then just keep a pointer to the socket as a member variable of the handler.

[craigt]

Ok fantastic!

Using setParent of the socket in ArdHandler seems to have sorted it out. I've run it a couple time for hundreds of cycles with no missing data. It makes sense actually. The socket is initialised from the handler's constructor which is in turn called from MainWindow so its essentially MainWindow code. I didn't even stop to consider that.

I think it is safer to not make the socket a member variable of the handler. If you want, then just keep a pointer to the socket as a member variable of the handler.

So would I then declare/intialise the socket in MainWindow, also move it to the QThread and provide a pointer to the handler?

Several times I've seen it recommended that the socket be declared and initialised in the run() of the QThread which will safely make it belong to the thread. It becomes a little hard to get access to it that way though.

You can do whatever you want as long as you make sure that a little-endian machine can read data sent by a big-endian machine and vice versa. And that different data alignment/structure packing on different platforms doesn't influence the data sent over the wire.

I am aware of this. I've left it out for now as I'm working on very similar platforms and testing the 2 programs mainly on the same machine. Some the data is coming through correctly so the endianess must be matched. It isn't going to change mid send as far as I understand, its normally fixed to a given platform. I will add this to the code once I get a basic prototype going.

[wysota]

Using setParent of the socket in ArdHandler seems to have sorted it out. I've run it a couple time for hundreds of cycles with no missing data. It makes sense actually. The socket is initialised from the handler's constructor which is in turn called from MainWindow so its essentially MainWindow code. I didn't even stop to consider that.

It's simpler than that. When you use moveToThread(), the object is moved together will all its children. However since the socket is not a child of the handler, it is kept in the thread that created the ArdHandler object.

So would I then declare/intialise the socket in MainWindow, also move it to the QThread and provide a pointer to the handler?

No, you can create the socket in constructor of the handler object and set the handler object as parent of the socket. Then when you move the handler, the socket will move together with it.

Several times I've seen it recommended that the socket be declared and initialised in the run() of the QThread which will safely make it belong to the thread. It becomes a little hard to get access to it that way though.

No, you can still a pointer to the socket in the containing object. However in your situation you don't have to subclass QThread at all.

I am aware of this. I've left it out for now as I'm working on very similar platforms and testing the 2 programs mainly on the same machine. Some the data is coming through correctly so the endianess must be matched.

If you use the same platform then they will match, however you should think about it already because later you will just forget about it and might run into trouble later.