QThread - Relation between quit, finished and deleteLater



  • I am sub-classing QThread and implementing a run method to perform some Task. I have few questions in cleanup of the sub-class object as below:

    I want to cleanup my sub-class (which derives from Qthread) in all the three cases:

    Thread has started and is inside the run loop, completed the task and done with the run method. Basically threads job is done.

    Thread is in the middle of the run loop and I want to cancel the tread.

    Thread is allocated with resource (my sub-class is created with new(). and assigned a task), but the thread has not yet started.

    I want to make sure that in all the above cases, I de-allocate my sub-class. So I associate finished() signal to deleteLater() slot. And in the cleanup function, which is called for all the above 3 cases, I just check if the sub-class is not NULL I just call sub-class->quit();.

    My question is what is the effect of quit() on an un-scheduled thread? Does it still call finished() signal? If it does not, then deleteLater() would not be called and obviously there would be a memory leak.

    Please help.

    -Sandeep



  • My advise would be don't subclass QThread, but rather use a worker object which you move to a QThread. This way handling all the above mentioned cases becomes a simple matter of signal slot connections and the checking of a flag inside your worker object.

    Your biggest problem using the subclassing approach is the case you mentioned in point 2. You could force cancel the thread using terminate() but that would leave you in an undefined state where you have no control over what's happening inside the run function. Alternatively you could just call quit and wait for the thread to finish its work, in that case the finish signal is emitted but the thread does not terminate immediatly.

    Point 1 and 3 should not be a problem.
    In point 1 the finished signal is emitted.
    In point 3 you could check if the thread isRunning() (should be false) and if the thread isFinished() (should be false as well). In this case you just need to delete the instance.



  • @#ifndef WORKER
    #define WORKER

    #include <QString>
    #include <QObject>

    class Worker : public QObject {
    Q_OBJECT

    public:
    Worker();
    ~Worker();
    quint64 dirSize(QString path);
    void setPath(QString path);

    public slots:
    void process();

    signals:
    void finished();

    private:
    QString path_;
    };

    #endif@

    @#include "Worker.h"
    #include <QDir>
    #include <QDirIterator>
    #include <QFileInfo>
    #include <iostream>

    Worker::Worker() {
    }

    Worker::~Worker() {
    }

    void Worker::setPath(QString path)
    {
    path_ = path;
    }

    void Worker::process() {
    std::cout << "The size of " << path_.toStdString() << " = " << dirSize(path_) << std::endl;
    emit finished();
    }

    quint64 Worker::dirSize(QString path)
    {
    quint64 sizex = 0;
    QDir dir(path);
    QDirIterator iterator(dir.absolutePath(), QDirIterator::Subdirectories);
    while (iterator.hasNext())
    {
    iterator.next();
    if (!iterator.fileInfo().isDir())
    {
    QFileInfo fInfo = iterator.fileInfo();
    sizex +=fInfo.size();
    }
    }

    return sizex;
    }
    @

    @#include <QApplication>
    #include <QThread>
    #include <iostream>
    #include <Worker.h>

    int main( int argc, char **argv )
    {
    QApplication a(argc, argv);

    QThread* thread = new QThread;
    Worker* worker = new Worker();
    worker->setPath("~/Desktop");  //Set your own path
    worker->moveToThread(thread);
    QObject::connect(thread, SIGNAL(started()), worker, SLOT(process()));
    QObject::connect(worker, SIGNAL(finished()), thread, SLOT(quit()));
    QObject::connect(worker, SIGNAL(finished()), worker, SLOT(deleteLater()));
    QObject::connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));
    thread->start();
    

    //sleep(10); //This line can be commented or un-commented to verify different cases

    if(thread->isRunning() && !thread->isFinished())
    {
        std::cout << "Thread is running " << std::endl;
    }
    
    if(!thread->isRunning() && !thread->isFinished())
    {
        std::cout << "Thread is yet to be scheduled " << std::endl;
    }    
    
    if(!thread->isRunning() && thread->isFinished())
    {
        std::cout << "Thread finished" << std::endl;
    }    
    
    std::cout <<"Exiting gracefully" << std::endl;
    

    return a.exec();
    }@

    New set of questions:

    Did I implement the code as you suggested me?

    Are the 3 if statements sufficient and the correct way to detect my 3 points (thread/job completed, thread/job cancelled, thread/job not scheduled) that I mentioned in original post?

    I am not doing any clean up in the if statements like "thread->quit() or thread->deleteLater() or worker->deleteLater(). This part is still quite confusing to me though I read qt docs + some blogs etc and also tried a bit of trial and error to get a feel of it.

    Could you please help me modify my code to gracefully handle clean-up of objects/threads in case of 3 scenarios(thread/job completed, thread/job cancelled, thread/job not scheduled).

    Thanks in advance,
    -Sandeep



  • When using the worker object approach you won't need the isRunning and isFinished checks.

    You should add a stop() slot to your worker object which just sets a boolean member var (which you also need to add). In your processing method you need to check this bool var and if it changed you need to end the processing and emit the finished signal of your worker object.

    BUT for this to work you also need to remove the while loop. This is neccessary so that the stop slot can get a chance to be triggered during the execution of the processing. So you need to implement a sort of signal slot loop in your worker object as a replacement for the while loop.



  • Another option would be to use "QtConcurrent::run()":http://qt-project.org/doc/qt-4.8/qtconcurrentrun.html for your dirSize(QString path) function. This way you don't need QThread and its easy to "cancel execution":http://qt-project.org/doc/qt-4.8/qfuture.html#cancel .


  • Moderators

    [quote author="KA51O" date="1380699495"]Another option would be to use "QtConcurrent::run()":http://qt-project.org/doc/qt-4.8/qtconcurrentrun.html for your dirSize(QString path) function. This way you don't need QThread and its easy to "cancel execution":http://qt-project.org/doc/qt-4.8/qfuture.html#cancel .[/quote]QtConcurrent::run() doesn't support cancelling, unfortunately. The bottom of your linked page says

    "Note that the QFuture returned by QtConcurrent::run() does not support canceling, pausing, or progress reporting. The QFuture returned can only be used to query for the running/finished status and the return value of the function."



  • Earlier I started with QtConcurrent, but later switched to sub-classing (because of no support for cancelling in QtConcurrent) from QThread, and finally I am using the above approach. I also found an interesting logic as explained in "Your text to link here...":http://fabienpn.wordpress.com/2013/05/01/qt-thread-simple-and-stable-with-sources/comment-page-1/#comment-64

    But I found some race-condition issues and proposed some corrections, I have left my comments in the above link. It would be great if one can review it.



  • [quote author="JKSH" date="1380699795"][quote author="KA51O" date="1380699495"]Another option would be to use "QtConcurrent::run()":http://qt-project.org/doc/qt-4.8/qtconcurrentrun.html for your dirSize(QString path) function. This way you don't need QThread and its easy to "cancel execution":http://qt-project.org/doc/qt-4.8/qfuture.html#cancel .[/quote]QtConcurrent::run() doesn't support cancelling, unfortunately. The bottom of your linked page says

    "Note that the QFuture returned by QtConcurrent::run() does not support canceling, pausing, or progress reporting. The QFuture returned can only be used to query for the running/finished status and the return value of the function."[/quote]

    Thank you for correcting me. I just saw that QFuture had a cancel function, didn't see the note in the QtConcurrent::run doc. Too bad its not possible.



  • It is quite logical that it is not posible. QtConcurrent::run runs a single function. There is no way that QtConcurrent can cancel that function in a sane way, as it would need to terminate the processing half way the function execution. When using QtConcurrent to process a collection of items, you can cancel by simply not starting the processing of the next item.



  • How does this function taken from "the blog post":http://fabienpn.wordpress.com/2013/05/01/qt-thread-simple-and-stable-with-sources/comment-page-1/#comment-64 allow the boolean _abort value to be changed during its for-loop? Is it the aditional QEventLoop created in the function?
    @
    void Worker::doWork()
    {
    mutex.lock();
    _abort = false;
    mutex.unlock();

    for (int i = 0; i < 60; i ++) 
    

    {

        mutex.lock();
        bool abort = _abort;
        mutex.unlock();
    
        if (abort) {
            break;
        }
    
        // This will stupidly wait 1 sec doing nothing...
        QEventLoop loop;
        QTimer::singleShot(1000, &loop, SLOT(quit()));
        loop.exec&#40;&#41;;
    
        emit valueChanged(QString::number(i&#41;&#41;;
    }
    

    emit finished();
    }
    @



  • @#include <QApplication>
    #include <QThread>
    #include <iostream>
    #include <Worker.h>

    int main( int argc, char **argv )
    {
    QApplication a(argc, argv);

    QThread* thread = new QThread;
    Worker* worker = new Worker();
    worker->setPath("/home/sandeep/Downloads");
    worker->moveToThread(thread);
    
    QObject::connect(thread, SIGNAL(started()), worker, SLOT(process()));
    QObject::connect(worker, SIGNAL(finished()), thread, SLOT(quit()), Qt::DirectConnection);
    
    thread->start();
    
    sleep(1);
    
    worker->abort();
    std::cout <<"----------------------Aborted-----------------Now waiting---------" << std::endl;
    thread->wait();
    delete thread;
    thread = NULL;
    
    delete worker;
    worker = NULL;
    
    
    std::cout <<"Exiting gracefully" << std::endl;
    

    return a.exec();
    }@

    @#include "Worker.h"
    #include <QDir>
    #include <QDirIterator>
    #include <QFileInfo>
    #include <iostream>

    Worker::Worker() {
    abort_ = false;

    }

    Worker::~Worker() {

    }

    void Worker::setPath(QString path)
    {
    path_ = path;
    }

    void Worker::process() {
    std::cout << "The size of " << path_.toStdString() << " = " << dirSize(path_) << std::endl;
    emit finished();
    }

    quint64 Worker::dirSize(QString path)
    {

    quint64 sizex = 0;
    QDir dir(path);
    QDirIterator iterator(dir.absolutePath(), QDirIterator::Subdirectories);
    
    while(iterator.hasNext()) 
    {
    
        // Checks if the process should be aborted
        mutex.lock();
        bool abort = abort_;
        std::cout << "Value of abort = " << abort << std::endl;
        mutex.unlock();
    
        if (abort) 
            break;
    
        iterator.next();
        if (!iterator.fileInfo().isDir())
        {
            QFileInfo fInfo = iterator.fileInfo();
            sizex +=fInfo.size();
            std::cout << "Progress = " << sizex << std::endl;
        }
    

    }

    return sizex;
    }

    void Worker::abort()
    {
    mutex.lock();
    abort_ = true;
    std::cout << "Able to set abort " << abort_ << std::endl;
    mutex.unlock();
    }
    @

    @#ifndef WORKER
    #define WORKER

    #include <QString>
    #include <QObject>
    #include <QMutex>

    class Worker : public QObject {
    Q_OBJECT

    public:
    Worker();
    ~Worker();
    quint64 dirSize(QString path);
    void setPath(QString path);
    void abort();

    public slots:
    void process();

    signals:
    void finished();
    void error(QString err);

    private:
    // add your variables here
    QString path_;
    bool abort_;
    QMutex mutex;
    };

    #endif@

    You can verify my argument using the above modified code. I guess, "process" function is executed in its own context and you can still call other functions in the worker class to set/rest your data, thus allowing a chance to cancel the thread.



  • [quote author="KA51O" date="1380620528"]My advise would be don't subclass QThread, but rather use a worker object which you move to a QThread. This way handling all the above mentioned cases becomes a simple matter of signal slot connections and the checking of a flag inside your worker object.

    Your biggest problem using the subclassing approach is the case you mentioned in point 2. You could force cancel the thread using terminate() but that would leave you in an undefined state where you have no control over what's happening inside the run function. Alternatively you could just call quit and wait for the thread to finish its work, in that case the finish signal is emitted but the thread does not terminate immediatly.

    Point 1 and 3 should not be a problem.
    In point 1 the finished signal is emitted.
    In point 3 you could check if the thread isRunning() (should be false) and if the thread isFinished() (should be false as well). In this case you just need to delete the instance.[/quote]

    How would you then have access to the QThread's methods? i.e. run, sleep, msleep etc.


  • Moderators

    [quote author="astodolski" date="1401365713"]How would you then have access to the QThread's methods? i.e. run, sleep, msleep etc.[/quote]Things you normally do in QThread::run() can be done by your worker QObject.

    sleep(), msleep() and usleep() are public static functions (in Qt 5). But, the whole point of using a worker objects + signal-slot connections is to write event-driven code, which does not need sleep() at all.

    If you don't want/need an event-driven architecture, then feel free to subclass QThread.

    The "Multithreading Technologies in Qt page":http://qt-project.org/doc/qt-5/threads-technologies.html gives some tips on how to choose the appropriate technique for your app (subclass QThread, worker QObjects, or something else completely).



  • Yes I absolutely need an event driven architecture. I don't understand why sub-classing QThread is so discouraged. It's as if it's bug prone.


  • Moderators

    [quote author="astodolski" date="1401367224"]Yes I absolutely need an event driven architecture.[/quote]Then I recommend using a non-subclassed QThread with worker QObjects.

    [quote]I don't understand why sub-classing QThread is so discouraged.[/quote]Subclassing QThread itself isn't discouraged -- see the link I posted above.

    Adding slots to subclassed QThreads is discouraged, because most people who did it had the wrong understanding of the QThread object, which causes to mysterious crashes. Then, they applied unintuitive workarounds.

    [quote]It's as if it's bug prone.[/quote]It is, unless the developer takes time to understand the concepts.

    This "blog post":http://blog.qt.digia.com/blog/2010/06/17/youre-doing-it-wrong/ was written by a Qt engineer who saw the same mistakes being committed again and again and again.

    EDIT: To be fair, here is a "blog reply":http://woboq.com/blog/qthread-you-were-not-doing-so-wrong.html by another Qt engineer, because people fought too hard against subclassing QThread



  • I suppose I have been fortunate to not have been subjected to the misuse of QThread. We do all background tasks in slots created in sub-classed QThreads. That is after all what is meant by an event driven architecture use is it not?


  • Moderators

    [quote author="astodolski" date="1401368454"]I suppose I have been fortunate to not have been subjected to the misuse of QThread. We do all background tasks in slots created in sub-classed QThreads. That is after all what is meant by an event driven architecture use is it not?[/quote]Yes, by "event-driven", I meant "use signals to do work in slots".

    How do you ensure that the slots are run in the correct threads?



  • Not sure if I understand the question. The work that runs in the other thread is called by the run method of QThread. That code runs after QThread::start is called by the GUI thread until the finished signal is emitted. The main thread just handles signal that are emitted from the secondary thread to do things like update progress bars and other UI related objects - pretty academic stuff I suppose.


  • Moderators

    [quote author="astodolski" date="1401375947"]Not sure if I understand the question. The work that runs in the other thread is called by the run method of QThread. That code runs after QThread::start is called by the GUI thread until the finished signal is emitted. The main thread just handles signal that are emitted from the secondary thread to do things like update progress bars and other UI related objects - pretty academic stuff I suppose.[/quote]That sounds fine. :)

    The main misuse of QThreads is this: Some people subclass QThread and add slots to the subclass. Then, when they connect a signal to it, they expect the slot to run in the secondary thread. That won't happen because the QThread object lives in the main thread, not the secondary thread.

    It was a very common mistake, so we now discourage people from adding slots to QThread subclasses. The easiest way to do this is simply to recommend: "If you want signal-slot connections between threads, don't subclass." This saves time, because newcomers are less likely to make mistakes, and we don't have to keep answering the same questions.

    However, if you understand how QThread works and you know what you're doing, you are free to ignore this recommendation and use whatever method you prefer (Rules have exceptions, after all). Just make sure you document this in your code, in case someone else inherits your project but doesn't understand QThread as well as you do.



  • That is a very good recommendation. I actually have seen mistakes made by users who use movetoThread is an incorrect way when using the recommended model.

    Thanks


Log in to reply
 

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