Question

This is my code. But I am confused where should I delete m_networkManager. I can do that in onRequestCompleted() slot but the problem is my program calls getData function frequently.

My fear is this case:

  1. getData() is called.
  2. Before onRequestCompleted() slot is fired, my program calls getData() again.
  3. onRequestCompleted() for the first getData() call gets fired.
  4. my program deletes m_networkManager which was actually allocated other memory when getData() was called second time.

I think this situation can cause crash.

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();
   reply->deleteLater();
   //m_networkManager->deleteLater();
}
Was it helpful?

Solution

Since you create new QNetworkAccessManager instance each time MyApp::getData() called, you need to manage to delete the right one in MyApp::onRequestCompleted() function. The problem in your code is that you always delete the last created QNetworkAccessManager and not the one that caused the slot call. My suggestion is following:

void MyApp::getData() {
   [..]
   // Not class member. Will be deleted in the slot.
   QNetworkAccessManager *networkManager = new QNetworkAccessManager(this);
   connect(m_networkManager, SIGNAL(finished(QNetworkReply*)),
           this, SLOT(onRequestCompleted(QNetworkReply *)));
   [..]
}

void MyApp::onRequestCompleted(QNetworkReply *reply) {
   QByteArray data = reply->readAll();
   reply->deleteLater();

   // Delete object whose signal triggered this slot.
   QObject *networkManager = sender();
   networkManager->deleteLater();
}
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top