Comments, Code and Qt. Some words about the wonderful world of software engineering

Why QNetworkAccessManager should not have the finished(QNetworkReply *) signal

I was recently writing some network code in Qt using QNetworkAccessManager and again I did the mistake I've already done a few times. The reason for my mistake was that QNetworkAccessManager provides the finished(QNetworkReply *) signal. There are essentially two ways to request some data from the net using QNetworkAccessManager. In the first approach you have the QNetworkAccessManager as an instance variable in your class and you connect the finished(QNetworkReply *) signal from the instance variable to your slot. This is a tempting solution as it provides a quick and easy solution to fetch some data from the net. void MyApp::getData() { QNetworkRequest request; request.setUrl(QUrl("http://www.domain.foo")); m_networkManager = new QNetworkAccessManager(this); // Instance variable connect(m_networkManager, SIGNAL(finished(QNetworkReply*)), this, SLOT(onRequestCompleted(QNetworkReply *))); m_networkManager->get(request); } void MyApp::onRequestCompleted(QNetworkReply *reply) { QByteArray data = reply->readAll(); } The second approach, which does exactly the same thing, is to connect the finished() signal from the QNetworkReply instance, which QNetworkAccessManager returns upon a call to get() or post() methods, to your slot. Note how you can also in this case get the QNetworkReply object that contains the completed request's data using the QObject::sender() method despite the fact that you don't get the result as a signal parameter. void MyApp::getData() { QNetworkRequest request; request.setUrl(QUrl("http://www.domain.foo")); m_networkManager = new QNetworkAccessManager(this); QNetworkReply *reply = m_networkManager->get(request); connect(reply, SIGNAL(finished()), this, SLOT(onRequestCompleted())); } void MyApp::onRequestCompleted() { QNetworkReply *reply = qobject_cast(sender()); QByteArray data = reply->readAll(); } So what's wrong with the first approach? Well - for example we could request some XML data from the Internet that is then read and parsed in the onRequestCompleted() slot. What happens if we now want to fetch subsequent data from the Internet based on the XML data in this slot? With the first approach you are still connected to the finished(QNetworkReply *) signal in the m_networkManager instance variable! You have changed the global state of your instance. It's easy to forget this. What  happens now is that when you fire the next network request, your first slot is called again even if you connect the finished(QNetworkReply *) signal from QNetworkAccessManager to another slot that you actually want to call when the subsequent request completes. Obviously, this is not what you want. One solution is, of course, to disconnect the signal in the onRequestCompleted() slot. But this is ugly and it's not easy to create event based or subsequent network requests based on data read from the Internet this way. The better solution is the second approach, where we connect to the finished() signal of the QNetworkReply instance. Now it becomes much clearer how we can chain multiple network requests based on previous network requests and construct application logic based on this. void MyApp::getSocialNetwork() { QNetworkRequest request; request.setUrl(QUrl("http://www.socialnetwork.foo")); m_networkManager = new QNetworkAccessManager(this); QNetworkReply *reply = m_networkManager->get(request); connect(reply, SIGNAL(finished()), this, SLOT(onRequestCompleted())); } void MyApp::onRequestCompleted() { QNetworkReply *reply = qobject_cast(sender()); QByteArray data = reply->readAll(); QString userId = parseUserIdFromData(data); QString statusUpdate("Hello, World!"); QNetworkRequest request; QString requestUrl = QString("http://www.socialnetwork.foo/%1/%2").arg(userId).arg(statusUpdate); request.setUrl(QUrl(requestUrl)); QNetworkReply *reply = m_networkManager->get(request); connect(reply, SIGNAL(finished()), this, SLOT(onStatusUpdateCompleted())); } void MyApp::onStatusUpdateCompleted() { // Do something when we have updated the status. } My guess is that the finished(QNetworkReply *) signal in QNetworkAccessManager is there for the sake of convenience. It's an obvious place to put a finished signal for a request that the manager does. But it can be dangerous too since you will probably use just one QNetworkAccessManager in your application which means that it cannot bind directly to your application states. I also think the finished(QNetworkReply *) signal should be removed from QNetworkAccessManager as it provides duplicate functionality to the finished() signal in QNetworkReply, but can also cause troubles, or at least uglier code, when you want to do multiple network requests based on previous network requests.

Technorati Tags: c++, network programming, qnetworkaccessmanager, Qt

Tagged as: c++, network programming, qnetworkaccessmanager, Qt Leave a comment