Solved Real confusion about when to delete QNetworkReply object
-
@JoeCFD said in Real confusion about when to delete QNetworkReply object:
Normally, you may do the following because it may finish immediately and the slot may not be called. Sigh!
Can you provide a citation? QNetworkAccessManager creates its own worker thread(s), and seems to use queued connections for communication with the worker objects that carry out the requests. I also don't see evidence of this extra check in the HTTP example
A demonstration that works for me (but proves nothing):
#include <QCoreApplication> #include <QNetworkAccessManager> #include <QNetworkReply> #include <QUrl> #include <QNetworkRequest> #include <QDebug> #include <QThread> int main(int argc, char *argv[]) { QCoreApplication a(argc, argv); QNetworkAccessManager manager; QUrl url("http://forum.qt.io"); QNetworkRequest request(url); qDebug() << "Fetching" << url; QNetworkReply *reply = manager.get(request); QThread::sleep(60); // Wait a minute! QObject::connect(reply, &QNetworkReply::finished, []() { qDebug() << "request finished"; QCoreApplication::instance()->quit(); }); return a.exec(); }
-
Yes, that means you don't need to call deleteLater() yourself. But you must still set your pointer to nullptr.
I only need to call it once in the constructor?
OR, avoid storing the QNetworkReply as a member variable.
The only reason I have it as a member variable is because I need a way to call
abort()
to cancel a download. -
@hbatalha said in Real confusion about when to delete QNetworkReply object:
@JoeCFD Thanks I understand now.
I didn't know that it could finish immediately and the slot may not be called. What would happen if that happens. I am curious.Read my code: if it finishes, you call the slot directly.
/* check reply is null or not first */
Why would we do something like that. Is there any chance that the QNetworkAccessManager::get() may return a nullptr?
Pure habit. -
@jeremy_k examples often show how things work. However, it may not include all you may need. I use it in posting and experienced issues.
-
@hbatalha said in Real confusion about when to delete QNetworkReply object:
Yes, that means you don't need to call deleteLater() yourself. But you must still set your pointer to nullptr.
I only need to call it once in the constructor?
You must set your pointer to
nullptr
every time the object is deleted. -
@JKSH This is how I did it
QObject::connect(&net, &QNetworkAccessManager::finished, [this, dest_file, url, fileDownloadRequestType](QNetworkReply* reply) { finished = true; if(! reply->error()) { QByteArray data = reply->readAll(); QFile file(dest_file); if( ! file.open(QIODevice::ReadWrite | QIODevice::Truncate)) Util::displayErrorMessage(tr("Something went wrong"), tr("Could not save the file"), this, true); file.write(data); file.close(); ui->msg_label->setText(tr("Download Complete")); ui->progress_label->setText(""); exe = dest_file; if(fileDownloadRequestType == FileDownloadRequestType::UPDATE) handleUpdateDownloadFinished(); } else if(reply->errorString() != "Operation canceled") { LOG_ERROR("Error Downloading from " + url + "\nDescription: " + reply->errorString()); reply = nullptr; Util::displayErrorMessage(tr("Network Error"), tr("Please try again later"), this, true); } m_reply = nullptr; });
-
@hbatalha But now you don't call deleteLater() on reply, right?
You need to call deleteLater() AND set the pointer to nullptr afterwards. -
@jsulm said in Real confusion about when to delete QNetworkReply object:
@hbatalha But now you don't call deleteLater() on reply, right?
You need to call deleteLater() AND set the pointer to nullptr afterwards.I didn't call it because I called
QNetworkAccessmanager::setAutoDeleteReplies()
in the constructor. -
Update:
So after running some tests and downloading multiple files I noticed a strange thing:
The slot connected to&QNetworkAccessManager::finished
gets called more than once if I give it some time and keeps getting called. And because I set thereply
to null in the first call, in the second call it overwrites the just downloaded file and writes null, making the FileDownloader pretty much useless.After some debugging I found out that because
FileDownloader::download
everyt ime is called with the same object in the MainWindow it theQNetworkAccessManager
object gets connected more than once and thus calling the slot more than once.I have a few options to solve this:
- Check
if ( nullptr != m_reply )
before connecting like @JoeCFD suggested in his answer - Disconnecting the
QNetworkAccessManager
object and connecting it again - Do not use lambda and create a method for that and connect it once in the constructor ( this is the option IMO but because I use some variables that come from the
FileDownloader::download
parameters inside the slot, it makes it difficult to use this option unless I created the member variables for those parameters.
- Check
-
If you have some stuff that is associated to the reply, you can use dynamic properties to keep everything in one place or use a QMap to store the reply specific data that you will then grab in your lambda.
-
you can use dynamic properties to keep everything in one place
Didn't understand what you meant
use a QMap to store the reply specific data that you will then grab in your lambda
Could you provide a minimal sample code on how to do that