Server programming - Best Practices

[wysota]

@wysota:
Yes, that's why I didn't go for the multi-threaded server. It has way too much overhead.

Overhead is not relevant here. If you handle a connection one second later, it won't compromise anything. Not reacting to bogus or unexpected data will.

Not getting data in is also on the list since the solution will have a mobile component to it and those devices can go off-grid any time. I figured that something along the lines of

void Server::onReadyRead() {
    QTcpSocket *client = qobject_cast<QTcpSocket *>(sender());
    if(!client)
        return;
    int blockSize = 0 ;
    QString incomingData;
    QDataStream in(client);
    in.setVersion(QDataStream::Qt_4_7);
 
    if (blockSize == 0) {
        if (client->bytesAvailable() < (int)sizeof(quint16))
            return;
        in >> blockSize;
    }
    if (client->bytesAvailable() < blockSize)
        return;
    in >> incomingData;
    doStuff(incomingData);
}

void Server::onReadyRead() {
    QTcpSocket *client = qobject_cast<QTcpSocket *>(sender());
    if(!client)
        return;
    int blockSize = 0 ;
    QString incomingData;
    QDataStream in(client);
    in.setVersion(QDataStream::Qt_4_7);

    if (blockSize == 0) {
        if (client->bytesAvailable() < (int)sizeof(quint16))
            return;
        in >> blockSize;
    }
    if (client->bytesAvailable() < blockSize)
        return;
    in >> incomingData;
    doStuff(incomingData);
}

To copy to clipboard, switch view to plain text mode 

should do the job. On top of that is a timer that let's each connection time out after a set period of time. (The client pings the server to confirm its status)


Thanks for the advice, it's really helpful to get a different perspective on things.

If you create a QDataStream in a slot connected to readyRead() then I can immediately tell you your code is incorrect without even reading it.

[Affenbrotbaum]

Excuse my ignorance wysota, but what's wrong with it? I've done testing on it and seems to work with multiple concurrent connections without leaking any memory.

[tbscope]

A QDataStream must be read in its entirety.
You create a new data stream for each chunk which will never work.

[wysota]

Excuse my ignorance wysota, but what's wrong with it? I've done testing on it and seems to work with multiple concurrent connections without leaking any memory.

It works only when you receive data you expect in a form you expect and amount you expect.

For example you are reading a block size and then discarding all data already read if there is not enough data available in the socket. Next time you get the signal when the rest of the data arrives but since you are not waiting for the data anymore, you will try to synchronize the data stream again which will obviously fail because there is some data from the previous block still left in the socket.

And provided you even fix it and have a buffer that will store all the incoming data before processing it, what if someone sends you a block size of 2GB? Are you going to buffer 2GB of data in your application for each incoming connection? What if you get 10 such connections? Do you have 20GB of memory in your machine?

If you have to use QDataStream, you synchronize it only once when the connection is established and you should closely monitor the status of the stream and at the first sign of lost synchro, you have to immediately drop the connection.

[Affenbrotbaum]

Hey

Alright I fixed it now and only one instance of the Datastream exists now per open Socket.
We are mainly dealing with small data transfer (< 5kb per hour per client). It automatically refuses all block sizes that are above a certain threshold. If the incoming data is incomplete waitForReadyRead() will take care of it. I'm doing some reworking right now, but I can post the code later.

Again, thanks for all the help. It's much appreciated
Alex

[wysota]

If the incoming data is incomplete waitForReadyRead() will take care of it.

That's a really bad idea. You are blocking your whole application while waiting (maybe indefinitely) for data from one client.

Again, don't focus on what you expect. Instead expect the unexpected.

[Affenbrotbaum]

Yeah I noticed that during testing and now I'm a little stomped. I guess there is no asynchronous implementation of that function ?

[wysota]

Sure there is. It's the readyRead() signal

[Affenbrotbaum]

Hehe ok now I'm confused . The above code in post #6 is what's connected to the readyRead() signal. Only 2 differences are a) The QDataStream now gets created once per socket when I'm working through the TcpServer::hasPendingConnections() list and b) I limit the data being received since I know all the strings and their length that are permitted to be submitted (socket->bytesAvailable()). I just had for testing purposes a waitForReadyRead() in there to see how it behaves with large or no data coming in.

So far, everything seems to be working now. I'll post some code this weekend so other people have an example of this whole conversation. Thank you very much for all the help

Cheers
Alex

[wysota]

The above code in post #6 is what's connected to the readyRead() signal.

That's correct and that's the right approach. But the code you put inside the slot connected to this signal is relevant.

b) I limit the data being received since I know all the strings and their length that are permitted to be submitted (socket->bytesAvailable()).

And that's your problem. You assume the data is going to look like you expect it. A good server is not one that behaves correctly for the data it expects, that's easy to achieve. A good server behaves correctly for the data (and behaviour) it doesn't expect.

I just had for testing purposes a waitForReadyRead() in there to see how it behaves with large or no data coming in.

So far, everything seems to be working now.

Such tests are useless.

With the code you posted so far I can compromise your whole server computer (not only your application) within seconds.