Solved Real confusion about when to delete QNetworkReply object
-
Use this to check if reply finishes before it is deleted.
connect( m_reply, &QNetworkReply::finished, **** );Normally, you may do the following because it may finish immediately and the slot may not be called. Sigh!
if ( nullptr != m_reply ) { /* check reply is null or not first */ if ( m_reply->isRunning() ) { /* connect when it is still running */ connect( m_reply, &QNetworkReply::finished, this, &getFinished() ); } else { /* not running(already finished) and call slot directly while connection is not needed */ getFinished(); } }
-
@SGaist said in Real confusion about when to delete QNetworkReply object:
You know that deleteLater does not change that pointer to a nullptr value ?
Yeah, which is making me confused on why it's crashing the app. Any ideas?
I made some changes in these two functions:
void FileDownloader::download(QString url, QString dest_file, QString downloadName, FileDownloadRequestType fileDownloadRequestType) { this->show(); downloadBegan = false; finished = false; connect(ui->pushButton, &QPushButton::clicked, [this] { this->close(); }); if(MAIN.preferences.theme == Preferences::Theme::DARK) { ui->msg_label->setStyleSheet("color : white;"); ui->progress_label->setStyleSheet("color : white;"); } else if(MAIN.preferences.theme == Preferences::Theme::LIGHT) { ui->msg_label->setStyleSheet("color : black;"); ui->progress_label->setStyleSheet("color : black;"); } ui->download_progressBar->setMinimum(0); ui->download_progressBar->setMaximum(0); ui->msg_label->setText(tr("Downloading ") + downloadName); reply = net.get(QNetworkRequest(QUrl(url))); downloadBegan = true; finished = false; if(QFile::exists(dest_file)) QFile::remove(dest_file); QObject::connect(reply, &QNetworkReply::downloadProgress, [this](qint64 received, qint64 total) { qDebug() << received << " of " << total; ui->progress_label->setText(Util::displaySize(received) + " of " + Util::displaySize(total)); ui->download_progressBar->setRange(0, total); ui->download_progressBar->setValue(received); }); QObject::connect(&net, &QNetworkAccessManager::finished, [this, dest_file, fileDownloadRequestType](QNetworkReply* reply) { finished = true; if(! reply->error()) { QByteArray data = reply->readAll(); reply->deleteLater(); 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); qDebug() << "Size written: " << file.write(data); file.close(); ui->msg_label->setText(tr("Download Complete")); ui->progress_label->setText(""); exe = dest_file; if(fileDownloadRequestType == FileDownloadRequestType::UPDATE) handleUpdateDownloadFinished(); } }); }
void FileDownloader::closeEvent(QCloseEvent *) { if( downloadBegan && ! finished) reply->abort(); }
Instead of using the reply object to check if the download has finished I am using
bool
variables. I call deleteLater in the slot. -
@JoeCFD I didn't quite understand the code
-
@hbatalha added comments.
-
We do something like this:
connect(reply, &QNetworkReply::finished, someobject, &SomeObject::someHandler); // or lambda connect(reply, &QNetworkReply::finished, reply, &QObject::deleteLater);
This keeps the lifetime of the reply separate from the "someobject".
-
@hbatalha said in Real confusion about when to delete QNetworkReply object:
@SGaist said in Real confusion about when to delete QNetworkReply object:
You know that deleteLater does not change that pointer to a nullptr value ?
Yeah, which is making me confused on why it's crashing the app. Any ideas?
- Most important: Set
FileDownloader::reply
tonullptr
after you call deleteLater(). - Second most important: Give your member variable a different name. Don't call it
reply
, since your lamba's parameter is also calledreply
. This causes shadowing (https://www.learncpp.com/cpp-tutorial/variable-shadowing-name-hiding/ )
- Most important: Set
-
For Qt 5.14 and later, theres QNetworkAccessmanager::setAutoDeleteReplies() which calls deleteLater() after emitting the finished signal.
-
@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./* 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?
-
@fcarney said in Real confusion about when to delete QNetworkReply object:
connect(reply, &QNetworkReply::finished, reply, &QObject::deleteLater);
So this is the same as @jeremy_k answer:
QNetworkAccessmanager::setAutoDeleteReplies()
? -
@jeremy_k So that will eradicate the need to call
deleteLater
? -
@hbatalha said in Real confusion about when to delete QNetworkReply object:
So that will eradicate the need to call
deleteLater
?Yes, that means you don't need to call
deleteLater()
yourself. But you must still set your pointer tonullptr
.OR, avoid storing the QNetworkReply as a member variable.
-
@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