Real confusion about when to delete QNetworkReply object

jsulm

@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.

hbatalha

@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

hbatalha

@SGaist

you 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 it reply is null or not before calling deleteLater

SGaist

What stack trace do you get when it crashes ?

hbatalha

@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();

SGaist

You know that deleteLater does not change that pointer to a nullptr value ?

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();
       }
   }

hbatalha

@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

@JoeCFD I didn't quite understand the code

JoeCFD

@hbatalha added comments.

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".

JKSH

@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 to nullptr 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 called reply. This causes shadowing (https://www.learncpp.com/cpp-tutorial/variable-shadowing-name-hiding/ )

jeremy_k

For Qt 5.14 and later, theres QNetworkAccessmanager::setAutoDeleteReplies() which calls deleteLater() after emitting the finished signal.

hbatalha

@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?

hbatalha

@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() ?

hbatalha

@jeremy_k So that will eradicate the need to call deleteLater?

JKSH

@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 to nullptr.

OR, avoid storing the QNetworkReply as a member variable.

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();
}

hbatalha

@JKSH

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.

JoeCFD

@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.