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

30Jul/1119

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: , , ,

  • http://blog.mardy.it Mardy

    Another important observation when you are doing many requests during the QNetworkAccessManager lifetime is to delete the QNetworkReply objects.

    • http://www.johanpaul.com/blog/ Johan Paul

      Thanks, yes! I omitted this in my code snips.

      And delete them with request->deleteLater()

  • Anonymous

    The api I’d really want is passing C++0x lambda to get()

    • http://www.johanpaul.com/blog/ Johan Paul

      I heard that Qt5 should utilize some features of C++0x. I also hope they will utilize lambda functions more in general and this for sure looks like a nice place to take advantage of them. 

  • http://twitter.com/minimoog77 Toni Jovanoski

    You can do (I have done it and have been using it) ‘recursive’ QNetworkReply::finished() : you can connect finished() signal to slot where you create new QNetworkReply connecting to the same slot. Interesting stuff.

    • http://www.johanpaul.com/blog/ Johan Paul

      How do you then track in this recursive slot the application state? Some instance global variable with if-elses, or…?

      • Toni Jovanoski

         It’s not recursive, it’s just look like it’s a recursive. Better name would maybe selfconnecting slot. :) It was about network data who has cursor/paging. So to receive next data you need previous cursor, and in the selfreceiving slot where you just received previous cursor, you make new network request which you connect to this so called selfreceiving slot.

        ‘sorry about different login, no twitter access for me in this place’

  • Denis Kormalev

    Not agreed fully. There are some cases when receiving QNetworkReply as parameter is better than using QNetworkReply::finished(). For example when you have some stateless network protocol with a lot of different commands. In this case you will have one place to manage calls to commands response handlers instead of handling it in all these small handler methods.

    • http://www.johanpaul.com/blog/ Johan Paul

      Well – if in that case your application logic or network code does not depend on the network data, then this would be ok. Also, as I said in my text, it’s convenient to get the network data with the first approach I presented if you really just want to read a blob of data and that’s it. But this is not always the case. I would even argue usually the case :)

      But, more importantly, you can do exactly what you stated above with the second approach without a need for multiple slots. So, even if you just want to read data from the network with a lot of different commands and call command handlers, you can do that in one single slot and connect to that slot from the QNetworkReply’s finished() signal (the second approach). In the slot you get the data with QNetworkReply *r = qobject_cast(QObject::sender()). This is exactly what you would get if you got the QNetworkReply *r as a signal parameter. 

      Why do I think the latter approach is better even if the end result is the same and you would not care about subsequent network calls? Because of API clarity. Just one approach is better than two approaches for some given use case. 

  • Mason Chang

    sender() is not a good practice. According to Qt’s document, developers should be careful when use sender().
    My solution is to use a member variable for a request, after getting into slot, call deleteLater and assign it to a NULL pointer. You’ll also make some additional efforts to avoid this pointer to be override by calling request again.

    • http://twitter.com/minimoog77 Toni Jovanoski

      I don’t care in this case if sender() is not good practice. What will you do if you have multiple network requests? How will you determine which QNetworkReply pointer is?

      • Dan

        Guy you are able to use QSignalMapper to correctly track what objects call which signal. Sender() is fine, but QSignalMapper would be better practice.

    • http://www.johanpaul.com/blog/ Johan Paul

      I’ve read this part of the documentation several times, and I can’t really agree with you. In what way would you say it’s not good practice? Qt documentation says

      “Warning: This function violates the object-oriented principle of modularity. However, getting access to the sender might be useful when many signals are connected to a single slot.”

      Sure – it’s maybe not as elegant as some other design pattern that you implement yourself, but in this QNetworkReply * use-case it seems to fit very well into this design. The documentation also says that this pointer will be invalid if the sender is destroyed when calling sender(), but again, in QNetworkAccessManager case this cannot be the case. And since QNAM is already async, I don’t have a need to put it into a separate thread either, so threads are not an issue for me in this case.

  • http://www.facebook.com/profile.php?id=1016927226 Andrea Martelli

    Great! This perfectly solved my issue! ;) Thanks a lot.

  • Jarrod

    You don’t need to connect and disconnect signals in the first approach, just store a variable in the request e.g.

    enum {
    REQUEST_TYPE = QNetworkRequest::User
    };

    QNetworkRequest request(endpoint);
    request.setAttribute((QNetworkRequest::Attribute)REQUEST_TYPE, REQ_RequestLogin);

    void WebAPI::requestFinished(QNetworkReply* a_reply) {
    switch(getRequestType(a_reply))
    {
    case REQ_RequestLogin:
    {

    ….

    }

    // etc.

    }

    I often prefer this approach to having a different slot for every different request.

    • http://www.johanpaul.com/blog/ Johan Paul

      This indeed sounds like a good approach as you then have only one place to handle all responses to all requests (for that class).

      I think this could be better documented in the Qt docs, though.

    • DrTebi

      @Jarrod
      This really helped, thank you. Using attributes makes it quite simple.
      In Python, with the details ommitted, this would be the idea:
      [...]
      self.qnam = QNetworkAccessManager(self)
      self.qnam.finished.connect(self.finished_request)
      [...]
      self.request = QNetworkRequest()
      self.request.setAttribute(1001, ‘basic’)
      self.request.setUrl(url)
      self.qnam.get(self.request)
      [...]
      self.request = QNetworkRequest()
      self.request.setAttribute(1001, ‘basic’)
      self.request.setUrl(url)
      self.qnam.get(self.request)
      [...]
      def finished_request(self, reply):
      response = reply.readAll()
      if reply.request().attribute(1001):
      # handle basic reply
      elif reply.request().attribute(1002):
      # handle detail reply

      • http://deshackra.com/ shackra sislock

        Well, I was managing the request “states/type” inside my custom QObject in a very different way! I’ll improve my code by using your approach, thanks! Here is a paste of my actual code if anyone is interested on how not to handle request types/reasons/status http://pastebin.com/DQfxtxV9

  • WU Long

    Solved my problem, thanks very much! I follow this example, but always get errors in my application!

    http://www.developer.nokia.com/Community/Wiki/Creating_an_HTTP_network_request_in_Qt