Restarting multiple QThreads results in a crash

Plastic.Jesus

Hi Everyone:

I've encountered a strange crash issue. We have some worker objects, each with it's own thread (HAS-A). Periodically, we are encountering a crash when the objects thread is restarted.

The call stack at the point of crash is in the property retrieval for objectName() of the worker object.

Point of crash

Callstack of crash

The crash in our use case is sporadic but reliable. It is also very annoying as it is in a customer facing embedded application.

I boiled it down to a very small program which reliably causes the identical crash in a few iterations.

The worker object looks like this:

#define SIMPLEWORKERTHREAD_H
#include <QObject>
#include <QThread>

class SimpleWorkerThread : public QObject
{
    Q_OBJECT
public:
    SimpleWorkerThread(const QString& name);

    void start();

private:
    QThread _thread;

signals:
    void finished();

private slots:
    void onThreadStarted();
    void onThreadFinished();
};

#endif // SIMPLEWORKERTHREAD_H

#include <QCoreApplication>
#include <QDateTime>
#include <QDebug>
#include <QTimer>

SimpleWorkerThread::SimpleWorkerThread(const QString& name) :
    QObject()
{
    SimpleWorkerThread::setObjectName(name);
    connect(&_thread, &QThread::started, this, &SimpleWorkerThread::onThreadStarted);
    connect(&_thread, &QThread::finished, this, &SimpleWorkerThread::onThreadFinished);
    moveToThread(&_thread);
}

void SimpleWorkerThread::start()
{
    _thread.start();
}

void SimpleWorkerThread::onThreadStarted()
{
    qDebug() << QString("%1 %2 Started").arg(QDateTime::currentDateTimeUtc().toString("yyyy-MM-ddThh:mm:ss.zzzZ")).arg(objectName());

    // I tried introducing a delay, but it fails exactly the same way
    static const int delay = 1000;
    if(delay > 0) {
        QTimer::singleShot(delay, &_thread, &QThread::quit);
    }
    else {
        _thread.quit();
    }
}

void SimpleWorkerThread::onThreadFinished()
{
    qDebug() << QString("%1 %2 Finished").arg(QDateTime::currentDateTimeUtc().toString("yyyy-MM-ddThh:mm:ss.zzzZ")).arg(objectName());
    emit finished();
}

It starts, then calls QThread::quit either through a timer, or immediately, failing identically in either configuration.

I then have a class responsible for starting and stopping a pool of these worker threads. It starts all the workers, and waits for the final one to join before starting the whole pool again. The code for the pool object is below.

#ifndef THREADPOOL_H
#define THREADPOOL_H
#include <QObject>

class SimpleWorkerThread;
class ThreadPool : public QObject
{
    Q_OBJECT
public:
    ThreadPool(int testCount, int threadCount);

    void start();

private:
    void die(const QString& message);

    int _testCount;
    int _finishedCount;

    QList<SimpleWorkerThread*> _threads;

private slots:
    void startTestCycle();
    void onChildFinished();
};

#endif // THREADPOOL_H

#include "threadpool.h"
#include "simpleworkerthread.h"

#include <QCoreApplication>
#include <QDebug>
#include <QTimer>

ThreadPool::ThreadPool(int testCount, int threadCount) :
    QObject(),
    _testCount(testCount)
{
    qDebug() << QString("Will perform %1 iterations with %2 threads").arg(testCount).arg(threadCount);
    for(int i = 0;i < threadCount;i++) {
        SimpleWorkerThread* thread = new SimpleWorkerThread(QString("Thread %1").arg(i + 1));
        connect(thread, &SimpleWorkerThread::finished, this, &ThreadPool::onChildFinished);
        _threads.append(thread);
    }
}

void ThreadPool::start()
{
    QTimer::singleShot(0, this, &ThreadPool::startTestCycle);
}

void ThreadPool::die(const QString& message)
{
    qDebug() << "FATAL: " << message;
    QCoreApplication::quit();
}


void ThreadPool::startTestCycle()
{
    qDebug() << "********** Starting next cycle " << _testCount << " left to go";

    _finishedCount = 0;

    // Start all the threads
    for(SimpleWorkerThread* thread : _threads) {
        if(thread->thread()->isRunning()) {
            // Sanity check.... this never happens
            die("Thread was still running");
        }
        thread->start();
    }
}

void ThreadPool::onChildFinished()
{
    SimpleWorkerThread* thread = static_cast<SimpleWorkerThread*>(sender());
    if(thread->thread()->wait(1000) == false) {
        // Sanity check.... this never happens
        die("Never joined");
    }

    _finishedCount++;
    if(_finishedCount >= _threads.count()) {
        if(--_testCount <= 0) {
            qDebug() << "All cycles complete successfully";
            QCoreApplication::quit();
        }
        else {
            QTimer::singleShot(0, this, &ThreadPool::startTestCycle);
        }
    }
}

main.cpp looks like this:

#include "threadpool.h"

#include <QCoreApplication>
#include <QDebug>

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);

    ThreadPool test(500, 10);
    test.start();
    a.exec();
    return 0;
}

It appears that there may be something in the Qt property mechanism which is not thread-safe, but this seems unlikely.

Our actual application is nowhere near as busy as the provided test code, but it reliably fails in the same way minutes or hours later.

I've reproduced the problem on Qt 6.2.3, 6.5.3 and 6.7.1.

The code is on my github at https://github.com/StevePunak/thread-restart-failure.git

I would really some other eyeballs on this. Thanks in advance.

Plastic.Jesus

A quick addendum:

Is it possible that my understanding that QThreads are restartable is incorrect?
My environment is Fedora 39 x64.

(I had to reply to myself as my attempt to edit the original post resulted in it being flagged as spam ;)

Christian Ehrlicher

I looked at this crash but have no clue what's going wrong here.
Therefore I've created a patch for you: https://bugreports.qt.io/browse/QTBUG-126134 - feel free to add more information about the problem there.

Plastic.Jesus

@Christian-Ehrlicher

Thanks for creating the bug!

Just to make sure that my understanding of QThread restartability is correct, it's true that QThreads should be restartable? I couldn't find a definitive statement in the docs about this, but it seems reasonable.

BTW, it seems that it only happens to be objectName() that first causes the failure. I initially thought the same thing and used a local variable to name the thread. Turns out, by removing the call to objectName(), it only defers the failure until some other QObject property is accessed. Seems like something is horked in the QObject itself. I will add that information to the bug report.

JoeCFD

@Plastic-Jesus

Can you try to add a func clear() to the worker class and move the timer out of worker?
When the timer has time out, call clear() of the worker.

void WorkerName::clear()
{
    emit finished();
}

Connect worker finished() to thread quit() when thread is added to the worker. This is how I stop the worker.

Christian Ehrlicher

@Plastic-Jesus said in Restarting multiple QThreads results in a crash:

Just to make sure that my understanding of QThread restartability is correct

It is correct. As Thiago told you you should rework your stuff to avoid the crash. Maybe use QThreadPool instead your own solution.

Ronel_qtmaster

@Plastic-Jesus I will just give some hints

When you call QThread::start() it calls the function run() of QThread class.so you should reimplement the function run() not start()
you should always consider deleting the thread when it has finished
When you run many threads on the same time, it can results in unsufficient memory to execute them.Therefore also think about that aspect.

Christian Ehrlicher

@Ronel_qtmaster said in Restarting multiple QThreads results in a crash:

When you call QThread::start() it calls the function run() of QThread class.so you should reimplement the function run() not start()

Please look at the code - QThread is not reimplemented anywhere so no need to override run()

you should always consider deleting the thread when it has finished

Why? You can reuse them, that's also the point of QThreadPool

When you run many threads on the same time, it can results in unsufficient memory to execute them.Therefore also think about that aspect.

It's a reproducer of the problem - no need to add additional error handling here for such kind of errors.

jeremy_k

What's the goal of QThread reuse, other than avoiding repeat assignments of QObject thread affinity?

Christian Ehrlicher

@jeremy_k Creating a new QThread/std::thread can be time consuming - therefore QThreadPool. But yes in general I would at least not create such stuff by myself and simply use the classes/toools provided.
The problem here is also not QThread but a race condition underneath.

Ronel_qtmaster

@Christian-Ehrlicher I was just giving some hints about threads in general.Also, even if you want to reuse a thread the proper way is the delete it first.And even threadpool that you mentioned automatically deletes runnable objects after the execution before you can restart the threadpool.
Please do not tell something is wrong without checking it. thread error.PNG
Even the problem about memory you can see it above.And it creates crashes as well.

Christian Ehrlicher

@Ronel_qtmaster said in Restarting multiple QThreads results in a crash:

even if you want to reuse a thread the proper way is the delete it first.

That's not a re-use... you delete one and create a new one with all the overhead involved. And that's not how QThreadPool is working.

Please do not tell something is wrong without checking it.

You do not understand what a testcase/reproducer is for ...

Ronel_qtmaster

@Christian-Ehrlicher by reuse i mean a new thread object doing the same process.Even you asumed that we do not need to delete a thread after its use which is wrong.Even QThreadpool is doing that.I have shown you the proof that the use of many threads can create sometimes memory problems, therefore a crash.

Christian Ehrlicher

@Ronel_qtmaster said in Restarting multiple QThreads results in a crash:

have shown you the proof that the use of many threads can create sometimes memory problems, therefore a crash.

I don't know what you're trying to explain here - the crash the OP reports has nothing to do with too many threads. He is only creating ten and is reusing them. So what are you trying to tell us? That you can't create an indefinitly amount of objects without getting an oom situation? Thx I'm aware if this since 30 years of programming.

Ronel_qtmaster

@Christian-Ehrlicher you earlier said that the information i wrote were false.Now you are saying you don't understand.

Christian Ehrlicher

I just don't understand what your posts help to resolve the issue. The Maintainer of the Qt thread stuff already said it has nothing to do with the threads but it's a locking problem within QProperty handling. The testcase just reveals it in a very good and fast way.

jeremy_k

@Christian-Ehrlicher said in Restarting multiple QThreads results in a crash:

@jeremy_k Creating a new QThread/std::thread can be time consuming

That's an interesting comparison. std::thread starts executing when it is instantiated. It doesn't have the ability to separate the equivalent of QThread::QThread() from QThread::start().

At a glance, there's a significant amount of code in the platform-specific implementations of QThread::start(), leading me to wonder how much avoiding additional calls to QThread::QThread saves. The underlying platform thread is (re)created during each successful call to QThread::start() in the Windows and unix implementations.

Plastic.Jesus

@Christian-Ehrlicher Just a quick note. I agree with everything you said. Regarding QThreadPool, the actual software does not use a thread pool like my demo testcase. It simply has threads that fire up periodically to perform some maintenance operations. I created the thread pool in the test code as the most succinct demonstration of the issue.

Thanks again.