Should deleteLater() be called from inside QThread::run()?

CJha

Hi, I have a WorkerThreads which are subclass of QThread and have overridden run() method inside which I perform the calculations, other than run() these workers have no other method. I start workers from main GUI as Worker->start(). My worker thread looks like this:

workerthread.h

class WorkerThread : public QThread
{
    Q_OBJECT

public:
    WorkerThread(QObject parent = nullptr);
    ~WorkerThread();
    std::atomic<bool> bQuit{false}; // set to true to stop worker

protected:
    void run() override;
}

workerthread.cpp

//... constructor
//... destructor

void WorkerThread::run()
{
    forever{
        if(bQuite.load(std::memory_order_acquire)){
            this->deleteLater(); // should I do this? is `this` pointer necessary here?
            return;
        }

        /* calculation part, I use a mutex and a wake-lock to here to avoid loop running all the time */
    }
}

And to stop and delete the WorkerThread object I do this from my main GUI thread: Worker->bQuit.store(true, std::memory_order_release); and then use a wake-lock to wake up the worker so that it can process the bQuit variable.
According to Qt document for QObject::deleteLater():

if deleteLater() is called on an object that lives in a thread with no running event loop, the object will be destroyed when the thread finishes.

Since I am using Worker->start() and not calling exec() I believe my WorkerThread falls in the category of not having an event loop. So, is calling deleteLater() from inside run() safe?

Christian Ehrlicher

Since there is no eventloop, deleteLater can not work. Use the normal way with signal/slots as explained in the documentation.

KroMignon

@CJha said in Should deleteLater() be called from inside QThread::run()?:

Since I am using Worker->start() and not calling exec() I believe my WorkerThread falls in the category of not having an event loop. So, is calling deleteLater() from inside run() safe?

Yes, you won't have an event loop here, but the thread affinity of this class will be the thread in which it has been created.
For my understanding of deleteLater(), this should ensure that the instance will be destroyed on next iteration in attached thread event loop.
I guess the instance has been created in main thread. So it should be destroyed on next iteration on main event loop.

Christian Ehrlicher

@KroMignon is correct - deleteLater() is simply calling QCoreApplication::postEvent().

CJha

@Christian-Ehrlicher What is the normal way? I have read the QThread documentation many times, most of it is about using worker-object approach. Are you talking about this:

From Qt 4.8 onwards, it is possible to deallocate objects that live in a thread that has just ended, by connecting the finished() signal to QObject::deleteLater().

That is I just connect the finished()signal to it's deleteLater() slot in the constructor of WorkerThread like this:

WorkerThread::WorkerThread(QObject* parent)
    : QThread(parent)
{
    connect(this, &WorkerThread::finished, this, &WorkerThread::deleteLater);
}

Christian Ehrlicher

@CJha correct

CJha

@KroMignon Thanks, yes the instance has been created in the main thread so I guess this approach will work.

kshegunov

A note: You're reinventing the wheel with that atomic, Qt already does this for you. Look here.

jeremy_k

If I'm reading it correctly, there is a race.

1] WorkerThread::run calls deleteLater for this (in the thread represented by WorkerThread).
2a] The WorkerThread object is deleted (in the thread the WorkerThread object thread it was created in or associated with).
2b] WorkerThread::run() finishes execution (in the thread represented by WorkerThread).

There's no guarantee that the main thread isn't going to wake up and execute 2a before 2b completes.

kshegunov

@jeremy_k said in Should deleteLater() be called from inside QThread::run()?:

If I'm reading it correctly, there is a race.

You are reading it correctly, but that since had changed (supposedly):

That is I just connect the finished() signal to it's deleteLater() slot in the constructor of WorkerThread like this:
WorkerThread::WorkerThread(QObject* parent)
    : QThread(parent)
{
    connect(this, &WorkerThread::finished, this, &WorkerThread::deleteLater);
}

jeremy_k

@kshegunov said in Should deleteLater() be called from inside QThread::run()?:

@jeremy_k said in Should deleteLater() be called from inside QThread::run()?:

If I'm reading it correctly, there is a race.

You are reading it correctly, but that since had changed:

I did miss that. Thanks.

Connecting signals and slots from outside the thread associated with a QObject always makes me nervous. QObject::connect is documented as thread safe, but it doesn't know that the sender and receiver are valid at the time of connection.

kshegunov

@jeremy_k said in Should deleteLater() be called from inside QThread::run()?:

Connecting signals and slots from outside the thread associated with a QObject always makes me nervous.

Good instinct. Threading makes me nervous to begin with and I sometimes muse about how fearless can people get ... ;)

Currently a client is in that particular conundrum:
You slap threads for no good reason and you don't look both ways before you jump, you end up tracing race conditions in a production environment. Not a fun thing to have to do ...