Real confusion about when to delete QNetworkReply object
-
I have a class FileDownloader that I have an instance of it in my main class and I use it to download some files by calling a method 'download'. But I am having a hard time understanding when to call deleteLater on the QNetworkReply object.
Here is the part of the code that I consider relevant. if something is unclear I can post some code or even the entire class:void FileDownloader::download(QString url, QString dest_file, QString downloadName, FileDownloadRequestType fileDownloadRequestType) { this->show(); connect(ui->pushButton, &QPushButton::clicked, [this] { this->close(); }); 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) { 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); // closes the dialog file.write(data); file.close(); ui->msg_label->setText("Download Complete"); ui->progress_label->setText(""); finished = true; exe = dest_file; if(fileDownloadRequestType == FileDownloadRequestType::UPDATE) handleUpdateDownloadFinished(); } // reply->deleteLater(); // if I call it here instead it crashes my app every once in a while }); } void FileDownloader::handleUpdateDownloadFinished() { ui->download_progressBar->setMinimum(0); ui->download_progressBar->setMaximum(0); ui->pushButton->setText(tr("Install Update")); disconnect(ui->pushButton, &QPushButton::clicked, nullptr, nullptr); connect(ui->pushButton, &QPushButton::clicked, this, &FileDownloader::install); } void FileDownloader::install() { QProcess::startDetached(exe); qApp->quit(); } void FileDownloader::closeEvent(QCloseEvent *) { if(reply != nullptr && downloadBegan && ! reply->isFinished()) { reply->abort(); } reply->deleteLater(); }
Right now I calling
reply->deleteLater();
in thecloseEvent
( I am really not sure if it is safe) which seems to be working fine. Before I was I calling it in the lambda function connected to theQNetworkAccessManager::finished
signal but that was causing my app to crash often.I have read through many forums and topics about this and found different approaches. The most common one I found was calling the
reply->deleteLater();
in a slot connected toQNetworkAccessManager::finished
signal like this:void FileDownloader::finished(QNetworkReply* reply) { QByteArray = reply->readAll(); reply->deleteLater(); //do other stuff }
Unless I am misunderstanding the doc, it says not to delete the object there.
Note: Do not delete the object in the slot connected to the errorOccurred() or finished() signal. Use deleteLater().
In some they use a
QScopedPointer
to store thereply
.So I am not sure which is the correct way of deleting the
reply
.@hbatalha said in Real confusion about when to delete QNetworkReply object:
Use deleteLater()
And this is exactly what you are doing, right? So, what is the problem?
-
@hbatalha said in Real confusion about when to delete QNetworkReply object:
Use deleteLater()
And this is exactly what you are doing, right? So, what is the problem?
Hi,
@jsulm the OP calls deleteLater at the wrong time and also keeps a local copy of the reply which is not needed and on which he might call deleteLater while the pointer is dangling.
@hbatalha you should call deleteLater in your reply once the request is finished so in your case in the lambda that is connected to the QNAM finished signal.
-
Hi,
@jsulm the OP calls deleteLater at the wrong time and also keeps a local copy of the reply which is not needed and on which he might call deleteLater while the pointer is dangling.
@hbatalha you should call deleteLater in your reply once the request is finished so in your case in the lambda that is connected to the QNAM finished signal.
@SGaist Ah, I was looking at
void FileDownloader::finished(QNetworkReply* reply) { QByteArray = reply->readAll(); reply->deleteLater(); //do other stuff }
which is not the actual code.
-
@SGaist Ah, I was looking at
void FileDownloader::finished(QNetworkReply* reply) { QByteArray = reply->readAll(); reply->deleteLater(); //do other stuff }
which is not the actual code.
wrote on 21 Jul 2021, 14:44 last edited by@jsulm said in Real confusion about when to delete QNetworkReply object:
which is not the actual code.
That was an example I created to show the most commom way I found being suggested to do in other forums
-
Hi,
@jsulm the OP calls deleteLater at the wrong time and also keeps a local copy of the reply which is not needed and on which he might call deleteLater while the pointer is dangling.
@hbatalha you should call deleteLater in your reply once the request is finished so in your case in the lambda that is connected to the QNAM finished signal.
wrote on 21 Jul 2021, 14:52 last edited byyou should call deleteLater in your reply once the request is finished
If I do that the app will ocasionally crash
the OP calls deleteLater at the wrong time and also keeps a local copy
In my code where should I call it then?
on which he might call deleteLater while the pointer is dangling.
After a download is complete the next time will always be close the dialog so I think there won't be a case where that pointer is dangling unless I am missing something.
But for for safety I could add condition to see itreply
is null or not before calling deleteLater -
What stack trace do you get when it crashes ?
-
wrote on 21 Jul 2021, 16:26 last edited by
@SGaist I get this
Which points to this line in the closeEvent ;
if(reply != nullptr && downloadBegan && ! reply->isFinished())
This is the connect slot used:
QObject::connect(&net, &QNetworkAccessManager::finished, [this, dest_file, fileDownloadRequestType](QNetworkReply* reply) { 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); file.write(data); file.close(); ui->msg_label->setText(tr("Download Complete")); ui->progress_label->setText(""); finished = true; exe = dest_file; if(fileDownloadRequestType == FileDownloadRequestType::UPDATE) handleUpdateDownloadFinished(); } });
The difference here I call deleteLater right after
QByteArray data = reply->readAll();
-
You know that deleteLater does not change that pointer to a nullptr value ?
-
wrote on 21 Jul 2021, 18:25 last edited by JoeCFD
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(); } }
-
wrote on 21 Jul 2021, 21:34 last edited by
@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. -
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(); } }
-
wrote on 21 Jul 2021, 22:41 last edited by fcarney
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".
-
@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.@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
-
wrote on 22 Jul 2021, 00:31 last edited by
For Qt 5.14 and later, theres QNetworkAccessmanager::setAutoDeleteReplies() which calls deleteLater() after emitting the finished signal.
-
wrote on 22 Jul 2021, 01:55 last edited by
@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?
-
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".
wrote on 22 Jul 2021, 01:56 last edited by@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()
? -
For Qt 5.14 and later, theres QNetworkAccessmanager::setAutoDeleteReplies() which calls deleteLater() after emitting the finished signal.
-
@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.
-
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(); } }
wrote on 22 Jul 2021, 06:00 last edited by jeremy_k@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(); }
11/31