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



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


  • Lifetime Qt Champion

    Hi,

    Can you show how exactly you are using QTemporaryFile ?



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


  • Moderators

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



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

    QTemporaryFile 
    file.open()
    

    into the QtConCurrent::Run part as well.



  • 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!



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



  • 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;
    }
    
    


  • 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));
    


  • @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:


  • Qt Champions 2016

    @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;
    };
    


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



  • 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 :)


  • Qt Champions 2016

    @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;
    };
    


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


  • Qt Champions 2016

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



  • 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));
    


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



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


  • Qt Champions 2016

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



  • @Vadi2 For me personally its a bit to much especially as your lambda function has a nested lambda in it.

    I probably would have made this a seperated class .



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

    and KDE dependencies are not an option

    Just to be clear, the KDE API was redesigned in version 5 and now only uses Qt Calls so using KArchive does not link your application in any way to the KDE environment or the Linux OS. I have cross platform applications using those APIs. KArchive clearly states that works on: Android, FreeBSD, Linux, MacOSX and Windows.

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

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

    I use KArchive so I did not test it but by looking at the documentation of the method, it makes me think it does:

    from http://doc.qt.io/qt-5/qbytearray.html#qUncompress

    If you want to use this function to uncompress external data that was compressed using zlib...


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.