QTcpSocket question

lamprosg

Greetings,

I'm trying to create a (my 1st) server - client application and I came across to this problem.

Server needs to send multiple files (addresses stored in a QList filelist) to the client. I do it like this:

@for (int j=0;j<filelist.size();j++)
{
QFileInfo fileinfo;
fileinfo.setFile(filelist.value(j));
int filesize = fileinfo.size(); //Individual file size in Bytes

        QByteArray *block = new QByteArray[filesize];

        QFile *file= new QFile&#40;filelist.value(j&#41;);
        file->open(QIODevice::ReadOnly);
        *block = file->readAll();

        clientConnection->write(*block);

        delete file;                                   //Free the heap
        delete [] block;

}@

Also sizes are already sent and known to the client, this is why i tried it this way.

My question is, when the client reads the data, the tcpSocket->bytesAvailable() will go up to the sum of the file sizes, or do they reset (bytesAvailable()=0) for every new file received.

Thanks.

alexander

why @QByteArray *block = new QByteArray[filesize];@
maybe
@QFile *file= new QFile(filelist.value(j));
file->open(QIODevice::ReadOnly);
QByteArray block = file->readAll();
clientConnection->write(block);@

alexander

the @tcpSocket->bytesAvailable()@ will go up to the sum of the file sizes

tony

Hi,

better send the size first, so you'll be safer about what you're reading.

well, better this, I guess:

@
QScopedPointer< QFile > file(new QFile(filelist.value(j)));
file->open(QIODevice::ReadOnly);
clientConnection->write(file->readAll());
@

so, no delete at all :)

tobias.hunger

Uhm... do you really want to load the complete file into memory to send it? What will your application do when somebody tries to send a blueray disk rip? You like your OOM Killer, don't you?

I assume that on the reciever side you are not acting till you see all bytes available either. All somebody needs to do is send you /dev/zero... and your server will run out of network buffers/storage. Voila, a classic remote DoS:-)

Usually you read the file in blocks of a certain size and send/receive one after the other (and limit the maximum number of connections, etc.).

tobias.hunger

Antonio: How about:
@
QFile file(filelist.value(j));
file.open(QIODevice::ReadOnly);
clientConnection->write(file.readAll());
@

Disclaimer: Do not use this code if you care about your hardware staying alive!

tony

Well, I fold, you win :D

Anyway, I was trying to write a code "near" the one proposed, and exception-safe for memleak.

Btw, you were able to show wonderfully how the life of a programmer can be very hard!!! :)

T.

lamprosg

bq. do you really want to load the complete file into memory to send it? What will your application do when somebody tries to send a blueray disk rip? You like your OOM Killer, don’t you?

Tobias has a point. I'll try to fix this by reading it into blocks, although I smell more problems coming in the client side :D

Thanks very much for the feedback so far!

P.S. QScopedPointer is a very interesting class

lamprosg

Here's another rookie question:

Let's say that someone does send a blue-ray disc. On the client side I suppose buffering data will be saved in the free ram memory. When it runs out of it what will happen?

The first data will be deleted in order for the last ones to fit the memory?

bytesAvailable() will still continue to go up?

I suppose I'll have to save the file in blocks here as well.
Opening the file to save data will eat my memory with its previous saved data as well? Or QDataStream will keep serializing the data in the file and saving it without me worrying about it?

@QFile *file = new QFile(filename);
file->open(QIODevice::WriteOnly);
QDataStream save(file);

for (int=0;i<numberofclocks;i++)
{
save.writeBytes(bytes,blockSize);
}@

Thanks

tobias.hunger

What happens when a system runs out of memory? It will try to swap out memory areas to disk, try to reclaim memory from all running applications, and as a very last measure it will select one application and kill it (that is the Out-Of-Memory (OOM) killer I mentioned earlier). The heuristics of the OOM killer are usually good enough to pick the right app in a "stupid developer tries to read a blueray rip into memory in one go" use case:-)

Why do you want to use a QDataStream here? Streams are great to handle typesafe data but are not really necessary when pushing raw bytes. Why don't you just do file->write(bytes, blockSize) directly? The code makes little sense anyway (where do bytes come from, etc.).

lamprosg

Well, to understand things better, here's my effort for the code (only code that matters), if you're in the mood of reading it :)

Server sending a list of files:
@
for (int j=0;j<filelist.size();j++)
{
QFileInfo fileinfo;
fileinfo.setFile(filelist.value(j));
int filesize = fileinfo.size(); //Individual file size in Bytes

QFile *file= new QFile(filelist.value(j));
file->open(QIODevice::ReadOnly);

//Sending the files in packets of 5MB (short of)

if (filesize < 5000000) //If file size is smaller than 5MB
{
char *data = new char[filesize];

   file->read(data, filesize);                             //Read it all and put it in *data

   clientConnection->write(data);
   delete data;                                            //Free memory

}
else
{
qint64 position=0;
for (int x=0; x<(int) filesize/5000000 ; x++)
{
char *data = new char[5000000]; //5MB block to send

         file->seek(position);                               //Go the previous position we were
         file->read(data, 5000000);                          //Read 5MB of data
         position = file->pos();                             //Get the position we're in

         clientConnection->write(data);                      //Send the packet
         delete data;                                        //Free the memory for the next 5MB
    }

int leftovers=0;
if ( filesize % 5000000 != 0 ) //If we have left-overs
{
leftovers = filesize % 5000000; //Left-over bytes
char *data = new char[leftovers];
file->seek(position);
file->read(data, leftovers);
clientConnection->write(data); //Send those too
delete data;
}
}
file->close();

delete file;
@

Client getting those files, (doesn't work, gets stuck in line 2 'return'):
@
if (tcpSocket->bytesAvailable() < totalfilesize) //Wait until everything is available
return;

    ui->fileslabel->setText("Saving..");

    for (int i=0;i<numberofitems;i++)                       //For each file
    {
        if (filesizes[i] < 5000000)                         //If file is smaller than 5MB (or so)
        {
            char *data = new char[filesizes[i]];

            QFile *file = new QFile&#40;filenames[i]&#41;;          //Create the file
            file->open(QIODevice::WriteOnly);

            tcpSocket->read(data,filesizes[i]);             //Read the data from the socket

            file->write(data,filesizes[i]);                 //Write it all at once
            file->close();

            delete file;
            delete data;
        }
        else
        {
            QFile *file = new QFile&#40;filenames[i]&#41;;
            file->open(QIODevice::WriteOnly);

            qint64 position=0;
            for (int x=0; x<(int) filesizes[i]/5000000 ; x++)
            {
                char *data = new char[5000000];

                tcpSocket->seek(position);                  //Go the position we were
                file->seek(position);

                tcpSocket->read(data, 5000000);             //Read 5 megs from the socket
                file->write(data, 5000000);                 //Write 5 megs in the file

                position = file->pos();                     //Get the position we are

                delete data;                                //Free the memory for the next 5 megs
            }

            if (filesizes[i] % 5000000 !=0)
            {
                int leftovers = filesizes[i] % 5000000;
                char *data = new char[leftovers];

                tcpSocket->seek(position);
                file->seek(position);                       //Go the position we were

                tcpSocket->read(data, leftovers);
                file->write(data, leftovers);               //Write the leftovers

                delete data;
            }

            file->close();
            delete file;

delete [] filenames; //Finally setting the memory free
delete [] filesizes;
@

goetz

For reading the file and sending the data you could use this:

@
int bufSize = 5000000;
QVector<char> readBuffer(bufSize);
foreach(const QString &fileName, filelist) {
QFile file(fileName);
if(!file.open(QIODevice::ReadOnly)) {
qDebug() << "Error on opening" << filename;
continue;
}

while( qint64 readLen = file&#40;readBuffer.data(&#41;, bufSize))
    clientConnection->write(readBuffer, readLen); // assuming clientConnection is a QIODevice

file.close();

}
@

See the "Blocking Fortune Client Example":http://doc.qt.nokia.com/qt-maemo-4.7/network-blockingfortuneclient.html for a prototype for your read function. Use QFile (no need for a heap allocation with new here, btw) and the write(data, len) functions for saving the received data.

tobias.hunger

codestein: Read up on network programming before continuing with this project of yours! Your receiver is just waiting for someone to bring down the whole machine. Network programming is tricky as it is easy to exploit any mistakes made remotely. So you need to know what you are doing or you will allow somebody to launch denial of service attacks against your machine.

The blocksize is much too high for most networks. I was thinking of a block size of maybe 64k or something! And you could get the same effect with much less code.

@
char * data[BLOCKSIZE];
quint64 filepos = 0;

while (filepos < file.size()) {
quint64 length = file.read(data, BLOCKSIZE);
filepos += length;
socket.write(data, length);
}
@

should do pretty much the same thing your code does.

There is no reason to seek() all the time. Reading etc. implicitly moves the position in the stream, so you are in the right position to read/write already.

@
if (tcpSocket->bytesAvailable() < totalfilesize) //Wait until everything is available
return;
@

So you send those 20 blueray disk rips in blocks, but at the receiver side you buffer all 20 of those in the network buffers before you start to write them out to disk? After making sure all the data of all files is already buffered in the receiver you then proceed to read the data in chunks. Why don't you just grab the whole thing in one go.

You should get the data from the OS as soon as possible. Read as much as possible whenever you get a readReady signal and save it out to files asap. As an optimization you should make sure that you write decently sized chunks of data to the file. It is painfully slow to append BLOCKSIZE times one byte to a file, and much faster to add 1 times BLOCKSIZE bytes.

lamprosg

I'm doing this small program for the sake of learning! Didn't know where to start so I went straight for the code, examples etc

bq. There is no reason to seek() all the time. Reading etc. implicitly moves the position in the stream, so you are in the right position to read/write already.

In the case of bytesAvailable, if I read 64k
@if (tcpSocket->bytesAvailable() < 64000)
return;
@

And then I want the other 64, I'll do it with the same code again and not like this, right?
@if (tcpSocket->bytesAvailable() < 128000)
return;
@

That was a major question I had.

Thanks for the replies!

tobias.hunger

bytesAvailable returns the number of bytes not yet read.

Actually I do not see any need to check tcpSocket->bytesAvailable(): In a slot triggered by readReady() you know there are bytes available. Just do something like this:

@
// const int BUFFERSIZE = 1024*1024;
// char * buffer[BUFFERSIZE];
// quint64 offset = 0;
// quint64 fileSize = 0;

void dataAvailable()
{
quint64 length = tcpSocket->read(buffer[offset], BUFFERSIZE - offset);
if (length + offset == BUFFERSIZE || length + fileSize == sizeOfFile[i]) {
file.write(buffer, BUFFERSIZE);
offset = 0;
fileSize += length;
// switch to next file if necessary!
} else {
offset += length;
}
}
@

goetz

There is no need to do the work in chunks of, say 64k. The operating system and Qt libs do some reasonable buffering for you, so there is no need to add another one and increase complexity of your code. Make it work correctly in the very first place, and, only if it is too slow, optimize it afterwards!

lamprosg

Greetings again!

Indeed after a number of debuggings I saw some descent buffering without coding it, smaller or arround 1 MB (many times much less). Probably best to put it in your code.

Still, I tried this code below, and I can't find any errors (downloading multiple files).
Seems to work for 1 file.. When the 1st file ends, it crashes where I put the asterisk. :O

@
qint64 savepos=0; //Position of bytes we're in
qint64 length=0; //Length of bytes read
int counter=0;
(bool) newfile=true;
qint64 buffer=1048576; // 1 MB
QFile *file;

        if (newfile == true)
        {
            file = new QFile&#40;filenames[counter]&#41;;         //Create the file
            file->open(QIODevice::WriteOnly);            //Open it
            newfile = false;
        }

        char *data = new char[buffer];
        length = tcpSocket->read(data, buffer);        //Read at max: 1MB
        file->write(data,length);
        savepos += length;
        ui->getprogressbar->setValue( (int) (savepos/filesizes[counter])*100);
        delete data;

      if (savepos == filesizes[counter])       //File size reached, saved and closing
      {
          file->close();
          delete file;
          counter++;
          savepos=0;                           //Reseting
          length=0;
          newfile = true;

          if (counter == numberofitems)
          {
              delete [] filenames;                                    //Setting the memory free
              delete [] filesizes;
              tcpSocket->disconnectFromHost();
          }
      }

@

goetz

Could you explain "crash" a bit more? Any kind of message on the console?

BTW: If you do data = new char[] you must call delete[] data afterwards, otherwise you have memory leak.

lamprosg

bq. If you do data = new char[] you must call delete[] data afterwards, otherwise you have memory leak.

You're totally right, how did I miss that?

Crash:
Signal name: SIGSEGV
Signal meaning: Segmentation fault

Points me to QAbstractSocketPrivate::resetSocketLayer

Now I'm thinking about it, it's possible with multiple files that savepos goes higher than the filesize after last reading of the first file (reads part of the 2d too) and savepos is never equal to filesize.. And goes all wrong from there

goetz

That sounds reasonable. We don't know what your "protocol" for the file transfer is. Maybe you should do it one by one, only sending a new file when the preceding has arrived completely.

On the other hand, if the equality never becomes true, you will write only one file that grows bigger and bigger. The segfault should not happen on the comparison line you indicated, but on the socket->read().