How do I get QTemporaryFile and QtConcurrent::run to play nicely?

Vadi2

I'd like the QTemporaryFile I give to QtConcurrent::run to stick around until the thread is done, but it would seem that QTemporaryFile goes out of scope and the corresponding file is deleted before the thread is even launched.

I like the convenience of QTemporaryFile, is there any way to make this work?

SGaist

Hi,

Can you show how exactly you are using QTemporaryFile ?

Vadi2

Yeah happily! I'm unzipping the file after I've downloaded it:

    connect(getReply,
            &QNetworkReply::finished,
            this,
            std::bind(
                    [=](QNetworkReply* reply) {
                        // don't do anything if there was an error
                        if (reply->error() != QNetworkReply::NoError) {
                            return;
                        }
                        QByteArray downloadedArchive = reply->readAll();

                        QTemporaryFile file;
                        if (!file.open()) {
                            return;
                        }
                        file.write(downloadedArchive);
                        file.close();

                        QTemporaryDir temporaryDir;
                        if (!temporaryDir.isValid()) {
                            return;
                        }

                        // perform unzipping in a worker thread so as not to freeze the UI
                        auto future = QtConcurrent::run(mpHost.data(), &Host::unzip, file.fileName(), QStringLiteral("%1/.config/mudlet/edbee/").arg(QDir::homePath()), temporaryDir.path());
                        auto watcher = new QFutureWatcher<bool>;
                        QObject::connect(watcher, &QFutureWatcher<bool>::finished, [=]() {
                            if (future.result() == false) {
                                qWarning() << "failed to unzip";
                                return;
                            }

                            loadEdbeeThemes(true);

                        });
                        watcher->setFuture(future);

//                        loadEdbeeThemes(true);
//                        theme_download_label->hide();

                        reply->deleteLater();
                    },
                    getReply));

The issue I'm running into is that Host::unzip reports the file is missing! It worked fine before I added QtConcurrent::run(). I suspect the QTemporaryFile goes out of scope and is thus deleted before the function can even start.

jsulm

@Vadi2 Yes, file goes out of scope and deletes the temp file. You could allocate QTemporaryFile on the heap instead of on the stack, but then you need to take care to delete it when not used anymore.

J.Hilk

@Vadi2 what @jsulm said, or move the file creation

QTemporaryFile 
file.open()

into the QtConCurrent::Run part as well.

Vadi2

Ohh, clever. I could create a lambda for my QtConcurrent::run that'll create the temporary file, run the unzip function, and still delete the file when it goes out of scope!

VRonin

You don't really need the QTemporaryFile at all, you can use the QByteArray directly (you can use QBuffer to connect QByteArray to the QIODevice Interface). Anyway you need to create on the heap (new and pass a parent just to be safe) and delete it in the slot connected to &QFutureWatcher<bool>::finished

Side note: is Host::unzip static and thread safe?

Vadi2

In theory I don't, but to keep it simple my unzip function accepts just the filepath to unzip.

Yeah, my function is const, so it should be thread safe, right?

bool Host::unzip(const QString &archivePath, const QString &destination, const QDir &tmpDir) const
{
    int err = 0;
    //from: https://gist.github.com/mobius/1759816
    struct zip_stat zs;
    struct zip_file* zf;
    zip_uint64_t bytesRead = 0;
    char buf[4096]; // Was 100 but that seems unduly stingy...!
    zip* archive = zip_open(archivePath.toStdString().c_str(), 0, &err);
    if (err != 0) {
//        zip_error_to_str(buf, sizeof(buf), err, errno);

        zip_error_t error;
        zip_error_init_with_code(&error, err);
        qDebug() << "unzip error:" << zip_error_strerror(&error);
        zip_error_fini(&error);

        return false;
    }

    // We now scan for directories first, and gather needed ones first, not
    // just relying on (zero length) archive entries ending in '/' as some
    // (possibly broken) archive building libraries seem to forget to
    // include them.
    QMap<QString, QString> directoriesNeededMap;
    //   Key is: relative path stored in archive
    // Value is: absolute path needed when extracting files
    for (zip_int64_t i = 0, total = zip_get_num_entries(archive, 0); i < total; ++i) {
        if (!zip_stat_index(archive, static_cast<zip_uint64_t>(i), 0, &zs)) {
            QString entryInArchive(QString::fromUtf8(zs.name));
            QString pathInArchive(entryInArchive.section(QLatin1Literal("/"), 0, -2));
            // TODO: We are supposed to validate the fields (except the
            // "valid" one itself) in zs before using them:
            // i.e. check that zs.name is valid ( zs.valid & ZIP_STAT_NAME )
            if (entryInArchive.endsWith(QLatin1Char('/'))) {
                if (!directoriesNeededMap.contains(pathInArchive)) {
                    directoriesNeededMap.insert(pathInArchive, pathInArchive);
                }
            } else {
                if (!pathInArchive.isEmpty() && !directoriesNeededMap.contains(pathInArchive)) {
                    directoriesNeededMap.insert(pathInArchive, pathInArchive);
                }
            }
        }
    }

    // Now create the needed directories:
    QMapIterator<QString, QString> itPath(directoriesNeededMap);
    while (itPath.hasNext()) {
        itPath.next();
        QString folderToCreate = QStringLiteral("%1/%2").arg(destination, itPath.value());
        if (!tmpDir.exists(folderToCreate)) {
            if (!tmpDir.mkpath(folderToCreate)) {
                zip_close(archive);
                return false; // Abort reading rest of archive
            }
            tmpDir.refresh();
        }
    }

    // Now extract the files
    for (zip_int64_t i = 0, total = zip_get_num_entries(archive, 0); i < total; ++i) {
        // No need to check return value as we've already done it first time
        zip_stat_index(archive, static_cast<zip_uint64_t>(i), 0, &zs);
        QString entryInArchive(QString::fromUtf8(zs.name));
        if (!entryInArchive.endsWith(QLatin1Char('/'))) {
            // TODO: check that zs.size is valid ( zs.valid & ZIP_STAT_SIZE )
            zf = zip_fopen_index(archive, static_cast<zip_uint64_t>(i), 0);
            if (!zf) {
                zip_close(archive);
                return false;
            }

            QFile fd(QStringLiteral("%1%2").arg(destination, entryInArchive));

            if (!fd.open(QIODevice::ReadWrite | QIODevice::Truncate)) {
                zip_fclose(zf);
                zip_close(archive);
                return false;
            }

            bytesRead = 0;
            zip_uint64_t bytesExpected = zs.size;
            while (bytesRead < bytesExpected && fd.error() == QFileDevice::NoError) {
                zip_int64_t len = zip_fread(zf, buf, sizeof(buf));
                if (len < 0) {
                    fd.close();
                    zip_fclose(zf);
                    zip_close(archive);
                    return false;
                }

                if (fd.write(buf, len) == -1) {
                    fd.close();
                    zip_fclose(zf);
                    zip_close(archive);
                    return false;
                }
                bytesRead += static_cast<zip_uint64_t>(len);
            }
            fd.close();
            zip_fclose(zf);
        }
    }

    err = zip_close(archive);
    if (err) {
        zip_error_to_str(buf, sizeof(buf), err, errno);
        return false;
    }

    return true;
}

Vadi2

Here is my working code! Seems to work, hope I don't have gaping flaws in it:

    connect(getReply,
            &QNetworkReply::finished,
            this,
            std::bind(
                    [=](QNetworkReply* reply) {
                        // don't do anything if there was an error
                        if (reply->error() != QNetworkReply::NoError) {
                            return;
                        }
                        QByteArray downloadedArchive = reply->readAll();

                        tempThemesArchive = new QTemporaryFile();
                        if (!tempThemesArchive->open()) {
                            return;
                        }
                        tempThemesArchive->write(downloadedArchive);
                        tempThemesArchive->close();

                        QTemporaryDir temporaryDir;
                        if (!temporaryDir.isValid()) {
                            return;
                        }

                        // perform unzipping in a worker thread so as not to freeze the UI
                        auto future =
                                QtConcurrent::run(mpHost.data(), &Host::unzip, tempThemesArchive->fileName(), QStringLiteral("%1/.config/mudlet/edbee/").arg(QDir::homePath()), temporaryDir.path());
                        auto watcher = new QFutureWatcher<bool>;
                        QObject::connect(watcher, &QFutureWatcher<bool>::finished, [=]() {
                            if (future.result() == false) {
                                qWarning() << "failed to unzip";
                                return;
                            }

                            loadEdbeeThemes(true);
                            theme_download_label->hide();
                            tempThemesArchive->deleteLater();
                        });
                        watcher->setFuture(future);
                        reply->deleteLater();
                    },
                    getReply));

VRonin

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

Yeah, my function is const, so it should be thread safe, right?

Wrong (as a simple example, imagine if the Host object gets deleted). Arguably I did not bother reading all your code and it might as well be thread safe if converted to static.

There are "less manual" solutions to handle compression anyway:

QuaZip for zip only
KArchive for zip, 7zip, tar, gz and ar

kshegunov

@VRonin said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

Wrong

Indeed, very wrong. I'd add here a very short example to illustrate, sufficed to say it has given me many sleepless nights debugging third-party code:

class SomeClass
{
public:
    void someMethod() const // Means neither thread-safe nor reentrant
    {
        // Do something that writes to the member and suddenly oooOOOPS!
    }

private:
     mutable SomeType member;
};

VRonin

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

In theory I don't, but to keep it simple my unzip function accepts just the filepath to unzip.

You can also use the built in qUncompress to unzip the data

Vadi2

I wish I could, but I've read that it doesn't deal with zip archives.

We also have had bad experience with QuaZip compiling on Windows, so we had to take it out - and KDE dependencies are not an option.

I have followed the good advice here and unzip is now like this:

static bool unzip(const QString &archivePath, const QString &destination, const QDir &tmpDir);

I didn't realise const function was not enforced by the compiler! I take it that static is?

Thanks for the help everyone on this :)

kshegunov

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

I didn't realise const function was not enforced by the compiler!

It is enforced, but in the example the member's declared as mutable, which I find pretty abominable to begin with. mutable basically means that the specific member can be modified from anywhere as it is "not part of the class interface". It's equivalent to stripping down the const manually, e.g.:

class SomeClass
{
public:
    void someMethod() const
    {
        SomeType & playingWithFireHere = const_cast<SomeType &>(member)
    }

private:
     SomeType member;
};

J.Hilk

@Vadi2
just a small note.

You never delete tempThemesArchive, nether after finishing the unzip nor after exiting because of an error. That'll leak memory.

kshegunov

Actually there's tempThemesArchive->deleteLater(); I see, I haven't checked the whole source though ...

Vadi2

Ohh oops - in case of an error, it would not have gotten deleted. I've fixed it now:

    connect(getReply,
            &QNetworkReply::finished,
            this,
            std::bind(
                    [=](QNetworkReply* reply) {
                        // don't do anything if there was an error
                        if (reply->error() != QNetworkReply::NoError) {
                            return;
                        }
                        QByteArray downloadedArchive = reply->readAll();

                        tempThemesArchive = new QTemporaryFile();
                        if (!tempThemesArchive->open()) {
                            return;
                        }
                        tempThemesArchive->write(downloadedArchive);
                        tempThemesArchive->close();

                        QTemporaryDir temporaryDir;
                        if (!temporaryDir.isValid()) {
                            return;
                        }

                        // perform unzipping in a worker thread so as not to freeze the UI
                        auto future =
                                QtConcurrent::run(mudlet::unzip, tempThemesArchive->fileName(), QStringLiteral("%1/.config/mudlet/edbee/").arg(QDir::homePath()), temporaryDir.path());
                        auto watcher = new QFutureWatcher<bool>;
                        QObject::connect(watcher, &QFutureWatcher<bool>::finished, [=]() {
                            if (future.result() == true) {
                                loadEdbeeThemes(true);
                            }

                            theme_download_label->hide();
                            tempThemesArchive->deleteLater();
                        });
                        watcher->setFuture(future);
                        reply->deleteLater();
                    },
                    getReply));

J.Hilk

@kshegunov you're right, but that applies only if the whole operation is is successful.

Vadi2

I'm happy with being able to keep all the code together using lambdas instead of having people jump around slot function definitions. Do others find this style readable? I'm sure there is room for improvement.

kshegunov

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

Do others find this style readable?

Not me. I don't write functions that are longer than 1 screen (about 100-120 lines) and I don't write into a lambda anything that's longer than few lines, but I'm old-fashioned.