QThread creation and cleanup

Amomum

I know that questions like this one were already asked multiple times but I still can't find an exact answer.
I have a Worker class derived from QObject. I create ans instance of QThread, move worker into new thread, connect some signals. In the worker init method I create a timer and I do some work periodically. This work should be done all the time, until the app is closed. To kill the thread I call quit in window's destructor and wait for it to quit.

My concern is that worker's desctuctor is never called. I know that I should not allocate any resources in the worker constructor (coz they will be allocated in the different thread), I do all my allocatoins in the init method and all my deallocations in worker's deleteLater slot (which I connected to threads quit signal). But I'm not sure if it's safe to just omit destructors call entirely, may be QObject needs to do something important?

Please tell me how should I do it right?

Here's the code:

auto worker = new Worker();

worker->moveToThread( &m_workerThread );

connect( worker, &Worker::finished, &m_workerThread, &QThread::quit );
connect( worker, &Worker::finished, worker, &Worker::deleteLater );
    
connect( &m_workerThread, &QThread::finished, &m_workerThread, &QThread::deleteLater );
connect( &m_workerThread, &QThread::finished, worker, &Worker::deleteLater );
connect( &m_workerThread, &QThread::started,  worker, &Worker::init );

m_workerThread.start();



MainWindow::~MainWindow()
{
    qDebug() << "Window deleted";

    m_workerThread.quit();
    m_workerThread.wait();

    delete m_ui;
}

I'm also a puzzled about how should I create worker and qthread? Should they be heap allocated? Or can I make them simple member variables? I can't set a parent to a worker class, since it gets moved to thread, but should a qthread have a parent?

raven-worx

@Amomum
QThread::quit() only quits it's event-loop -> means when you used QThread::exec() in the run() method.
QThread::wait() just waits, nothing more.

So how does your run() implementation look like? You need to make sure that the thread finishes (returns from run()) at some point, since you already connected it's finished signal to deleteLater().

kshegunov

@Amomum said in QThread creation and cleanup:

My concern is that worker's desctuctor is never called.

It should be, when you return control to the thread's event loop (or after the finished() signal). How do you test it exactly?

I know that I should not allocate any resources in the worker constructor (coz they will be allocated in the different thread)

Well, this is wrong. You can allocate whatever you want in the constructor, but you should set the correct parent to your QObject instances, so they get moved with it to the correct thread.

I do all my allocatoins in the init method

This is also fine.

all my deallocations in worker's deleteLater slot (which I connected to threads quit signal)

This is suspicious. deleteLater isn't virtual so you probably have overloaded the QObject's slot and thus you don't get your object deleted. Please show a snippet for this piece of your code.

But I'm not sure if it's safe to just omit destructors call entirely

No. It may be safe, but you should take care to clean up your stuff in any case.

What you posted looks perfectly okay.
Note that connect( worker, &Worker::finished, worker, &Worker::deleteLater ); is superfluous, however.

I'm also a puzzled about how should I create worker and qthread? Should they be heap allocated?

Not necessarily, but heap allocation will work okay the way you've done it.

but should a qthread have a parent

Unless you intend to delete it manually (which you do with deleteLater), yes, you should set a parent to it. However, since you create the thread with auto-storage (on the stack) you shouldn't delete it to begin with, the stack will take care to free the memory.

J.Hilk

@Amomum hi,

Your thought process is solid, but the exmple you posted has a at least one issue that I see:

you delete your worker and your thread object when worker emit finished -> OK
but in your destructor you call again quit and wait on an object that is potentially already deleted.

You can correct that easy enough with a lambda:

connect( worker, &Worker::finished, worker, [=]{
     worker->deleteLater();
     m_workerThread.quit();
     m_workerThread = Q_NULLPTR;
});

//instead of 
connect( worker, &Worker::finished, &m_workerThread, &QThread::quit );
connect( worker, &Worker::finished, worker, &Worker::deleteLater );

MainWindow::~MainWindow()
{
    qDebug() << "Window deleted";

if(m_workerThread){
    m_workerThread.quit();
    m_workerThread.wait();
}

    delete m_ui;
}

There are circular connections bound to your deleteLater stuff, but that should be fine, as Qt removes/disconnects connections whose ref-Object is deleted.

Amomum

@raven-worx

QThread::quit() only quits it's event-loop -> means when you used QThread::exec() in the run() method.
QThread::wait() just waits, nothing more.

So how does your run() implementation look like? You need to make sure that the thread finishes (returns from run()) at some point, since you already connected it's finished signal to deleteLater().

I'm not inheriting from QThread, I'm using it as is, so I have no run() implementation.

@kshegunov

It should be, when you return control to the thread's event loop (or after the finished() signal). How do you test it exactly?

I put a qDebug call in it and I put a breakpoint in it. Nothing happens.

Well, this is wrong. You can allocate whatever you want in the constructor, but you should set the correct parent to your QObject instances, so they get moved with it to the correct thread.

Oh, so I just have to make them children of my worker. Okay, that's good to know.

This is suspicious. deleteLater isn't virtual so you probably have overloaded the QObject's slot and thus you don't get your object deleted. Please show a snippet for this piece of your code.

Probably that was my mistake. My deleteLater is just this:

void Worker::deleteLater()
{
    qDebug() << "Worker delete later";
}

I just commented out this deleteLater and workers destructor is now called. I think that was the issue, thank you!

Unless you intend to delete it manually (which you do with deleteLater), yes, you should set a parent to it. However, since you create the thread with auto-storage (on the stack) you shouldn't delete it to begin with, the stack will take care to free the memory.

I'm not intending to delete thread itself in deleteLater, only resources that I will allocate in said thread (i.e. in workers constructor or init).

@J-Hilk

you delete your worker and your thread object when worker emit finished -> OK
but in your destructor you call again quit and wait on an object that is potentially already deleted.

Hmmm you are correct. Connecting workers finished and threads quit was a mistake by copy-pasting. Any way, my worker won't emit finished (and it doesn't right now), so the problem lies elsewhere.

raven-worx

@Amomum said in QThread creation and cleanup:

I'm not inheriting from QThread, I'm using it as is, so I have no run() implementation.

?????
Then where does m_workerThread come from?!

Amomum

@raven-worx said in QThread creation and cleanup:

?????
Then where does m_workerThread come from?!

That's just a QThread object inside of my MainWindow class.
I'm not inheriting from QThread, I create a QThread object, worker object and I move worker into qthread. As far as I know that's one of the recommended ways of using qthread and since I need a thread that will live as long as the whole app does (and probably will need to handle events) I chose it instead of inhereting from qthread and reimplementing run().

kshegunov

@Amomum said in QThread creation and cleanup:

Oh, so I just have to make them children of my worker.

Indeed. So you can still create your timer in the constructor of the worker, just set its parent to the worker object. I usually also opt for using the stack for this, cause, well I'm a stack fan ... ;)
E.g.

class Worker : public QObject
{
    Q_OBJECT

public:
    Worker()
        : QObject(), timer(this) //< It's enough to set the parent so the timer be moved to the thread of the worker object
    {
    }

private:
    QTimer timer;
};

I'm not intending to delete thread itself in deleteLater, only resources that I will allocate in said thread (i.e. in workers constructor or init).

Well, then put whatever you want to clean up in the destructor. If that's not sufficient for some reason (although it'd be my preference), you can create your own slot, for example public slots: void Worker::freeResources(); and connect that to the finished() signal instead. deleteLater() is when you need the object to be deleted, so don't use it in this case.

@Amomum said in QThread creation and cleanup:

@J-Hilk
but in your destructor you call again quit and wait on an object that is potentially already deleted.

Connecting workers finished and threads quit was a mistake by copy-pasting.

No, that's fine. The worker may want to quit ahead of time. And there is no problem with this approach. The only issue is you're requesting your thread object be deleted through the deleteLater slot, which will cause segfault, because you'd be trying to delete an object that's created on the stack.

Amomum

@kshegunov said in QThread creation and cleanup:

No, that's fine. The worker may want to quit ahead of time. And there is no problem with this approach. The only issue is you're requesting your thread object be deleted through the deleteLater slot, which will cause segfault, because you'd be trying to delete an object that's created on the stack.

Hmm. So if I create my Qthread object on the stack (more precisely, as a member of MainWindow, but mainwindow object is created on the stack of main), I should not call quit() and wait(), I can just do nothing and Qthread dtor will be called automatically after main will exit (and it will call workers dtor)? Will that be okay?

Amomum

@kshegunov
Oh, I got it, sorry. I need to quit the thread and wait for it to quit. The problem will be if my worker will emit finished, thread will quit and then I close my mainwindow and try to quit the thread again. I shouldn't do that.
Thank you!

kshegunov

@Amomum said in QThread creation and cleanup:

I need to quit the thread and wait for it to quit.

Yes, you do, this is not optional.

The problem will be if my worker will emit finished, thread will quit and then I close my mainwindow and try to quit the thread again. I shouldn't do that.

This is not a problem. You can quit muiltiple times safely and wait() will block only if the thread's running.