Infinite wait when stopping QThread event loop and then calling wait

[embeddedmz_2]

Hi,

In my program, I launch a QThread to execute a method "startStressTest" like this :

void Application::onStartStressTest(int testsCount, int tPeriod, int t2Period, int t3Period, QString outputFolder)
{
    if (m_stressTestThread != nullptr || m_stressTestManager != nullptr)
    {
        // something is wrong here
        return;
    }

    m_stressTestThread = new QThread(this);
    m_stressTestThread->setObjectName("StressTestThread");
    m_stressTestManager = new StressTestManager();

    QObject::connect(m_stressTestManager, &StressTestManager::error,
        this, &Application::showError);
    QObject::connect(m_stressTestManager, &StressTestManager::progress,
        this, &Application::setStressTestProgress);
    QObject::connect(m_stressTestManager, &StressTestManager::stressTestCompleted,
        this, &Application::onStressTestFinished);

    m_stressTestManager->moveToThread(m_stressTestThread);
    m_stressTestThread->start();

    emit enableUi(false);
    //emit stressTestprogress("Starting stress testing", 0);
    m_isStressTesting = true;
    emit startedStressTesting();
    emit stressTesting(m_isStressTesting);

    QMetaObject::invokeMethod(m_stressTestManager, "setKeepStressTesting", Qt::QueuedConnection,
        Q_ARG(bool, true));
    
    QMetaObject::invokeMethod(m_stressTestManager, "startStressTest", Qt::QueuedConnection,
        Q_ARG(int, testsCount),
        Q_ARG(int, tPeriod),
        Q_ARG(int, t2Period),
        Q_ARG(int, t3Period),
        Q_ARG(QString, outputFolder));
}

In this method, I use a for loop. If the user wants to stop the loop by pressing a button, I have to use a method that processes events of the QThread event loop in the for loop condition :

void StressTestManager::startStressTest(int testsCount, int tPeriod, int t2Period, int t3Period, QString outputFolder)
{
    emit progress("Starting Valves Stress Test", 0);

    StressTestStatus testStatus = StressTestStatus::Completed;

    Logger* const logger = Application::instance()->logger();

    const int t1Period = (tPeriod - t2Period - t3Period) / 2;
    const int s1Period = t1Period;
    const int s2Period = t2Period;
    const int s3Period = t1Period;
    const int s4Period = t3Period;

    CanBusManager* cbm = Application::instance()->canBusManager();

    if (!outputFolder.isEmpty())
    {
        QObject::connect(cbm, qOverload<double, double, double>(&CanBusManager::boardMeasuresReceived),
            this, [this](double supp24v, double supp120v, double temperature) {
                m_Internals->BoardMeasures.push_back(BoardMeasures(
                    supp24v, supp120v, temperature, QDateTime::currentMSecsSinceEpoch()));
            }, Qt::QueuedConnection);

        QObject::connect(cbm, &CanBusManager::valveDiagIUMeasuresReceived,
            this, [this](int valveId, double current, double voltage) {
                if (valveId < 0 || valveId > 1)
                {
                    return;
                }

                m_Internals->ValveDiagMeasures[valveId].push_back(ValveIUMeasures(
                    current, voltage, QDateTime::currentMSecsSinceEpoch()));
            }, Qt::QueuedConnection);

        QObject::connect(cbm, &CanBusManager::valveIUMeasuresReceived,
            this, [this](int valveId, double current, double voltage) {
                if (valveId < 0 || valveId > 1)
                {
                    return;
                }

                m_Internals->ValveMeasures[valveId].push_back(ValveIUMeasures(
                    current, voltage, QDateTime::currentMSecsSinceEpoch()));
            }, Qt::QueuedConnection);

        QObject::connect(cbm, &CanBusManager::valveRLMeasuresReceived,
            this, [this](int valveId, double r, double l, double rDiag, double lDiag) {
                if (valveId < 0 || valveId > 1)
                {
                    return;
                }

                m_Internals->ValveRLMeasures[valveId].push_back(ValveRLMeasures(
                    r, l, rDiag, lDiag, QDateTime::currentMSecsSinceEpoch()));
            }, Qt::QueuedConnection);

        QObject::connect(cbm, &CanBusManager::boardTrapsReceived,
            this, [this](TrapsItem trap) {
                m_Internals->BoardTrips.push_back(TripsItemDated(
                    trap, QDateTime::currentMSecsSinceEpoch()));
            }, Qt::QueuedConnection);

        QObject::connect(cbm, &CanBusManager::valveDiagTrapsReceived,
            this, [this](int valveId, TrapsItem trap) {
                if (valveId < 0 || valveId > 1)
                {
                    return;
                }
                
                m_Internals->ValveTrips[valveId].push_back(TripsItemDated(
                    trap, QDateTime::currentMSecsSinceEpoch()));

            }, Qt::QueuedConnection);
    }

    QElapsedTimer sequencesStopWatch;
    QElapsedTimer maintainValvesLevelStopWatch;
    for (size_t test = 0; test < testsCount && /*!testError &&*/ !isCanceled(); ++test)
    {
        emit progress("", int(float(test) / (testsCount - 1) * 100));
        //emit error(QString("Acquisition %1/%2 timed out !").arg(loop).arg(numberOfLoops));

        // pour éviter de saturer le tampon d'envoi du Peak CAN
        bool sendFrames = true;

        /* S1 */
        sequencesStopWatch.start();
        do
        {
            if (sendFrames)
            {
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        0, true)));
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        1, false)));

                maintainValvesLevelStopWatch.start();
            }

            sendFrames = maintainValvesLevelStopWatch.hasExpired(50); // 50ms
        } while (!sequencesStopWatch.hasExpired(s1Period) && !isCanceled());

        if (!m_Internals->KeepStressTesting)
        {
            break;
        }

        // S2
        sendFrames = true;
        sequencesStopWatch.start();
        do
        {
            if (sendFrames)
            {
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        0, true)));
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        1, true)));

                maintainValvesLevelStopWatch.start();
            }

            sendFrames = maintainValvesLevelStopWatch.hasExpired(50); // 50ms
        } while (!sequencesStopWatch.hasExpired(s2Period) && !isCanceled());

        if (!m_Internals->KeepStressTesting)
        {
            break;
        }

        // S3
        sendFrames = true;
        sequencesStopWatch.start();
        do
        {
            if (sendFrames)
            {
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        0, false)));
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        1, true)));

                maintainValvesLevelStopWatch.start();
            }

            sendFrames = maintainValvesLevelStopWatch.hasExpired(50); // 50ms
        } while (!sequencesStopWatch.hasExpired(s3Period) && !isCanceled());

        if (!m_Internals->KeepStressTesting)
        {
            break;
        }

        // S4
        sendFrames = true;
        sequencesStopWatch.start();
        do
        {
            if (sendFrames)
            {
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        0, true)));
                QMetaObject::invokeMethod(cbm, "writeFrame", Qt::BlockingQueuedConnection,
                    Q_ARG(PH66Frame, PH66ActivateValveRequestFrame(cbm->lastAddress(),
                        1, true)));

                maintainValvesLevelStopWatch.start();
            }

            sendFrames = maintainValvesLevelStopWatch.hasExpired(50); // 50ms
        } while (!sequencesStopWatch.hasExpired(s3Period) && !isCanceled());

        if (!m_Internals->KeepStressTesting)
        {
            break;
        }

        dumpData(outputFolder); // data will be dumped into CSV files when the buffers reach a certain size
    }

    // force data dump
    if (!outputFolder.isEmpty())
    {
        emit progress("Dumping the rest of PH66 data started", 100);
    }
    processEvents();
    dumpData(outputFolder, true);
    if (!outputFolder.isEmpty())
    {
        emit progress("Dumping the rest of PH66 data finished", 100);
    }

    if (!m_Internals->KeepStressTesting)
    {
        testStatus = StressTestStatus::StoppedByUser;
    }

    emit stressTestCompleted(testStatus);
}

bool StressTestManager::isCanceled()
{
    processEvents();
    return !m_Internals->KeepStressTesting;
}

void StressTestManager::processEvents()
{
    auto const dispatcher = QThread::currentThread()->eventDispatcher();
    if (!dispatcher)
    {
        Application::instance()->logger()->log(LogLevel::critical, LogSource::all,
            "Stress test thread hasn't a dispatcher !");
        
        return;
    }
    dispatcher->processEvents(QEventLoop::AllEvents);
}

The CAN Bus manager lives in a dedicated thread.

I checked that void StressTestManager::startStressTest completes its execution but its QThread is still active :

and this is the call stack of the main thread that is waiting for the stress test thread to finish :

I think there's a deadlock and I don't know why. Maybe because I emitted signals (like emit stressTestCompleted(testStatus)) before the end of the startStressTest.

void Application::onStopStressTest()
{
    if (m_stressTestThread == nullptr || m_stressTestManager == nullptr)
    {
        return;
    }

    QMetaObject::invokeMethod(m_stressTestManager, "setKeepStressTesting",
        Qt::QueuedConnection, Q_ARG(bool, false));

    m_stressTestThread->quit();
    m_stressTestThread->wait();

    delete m_stressTestManager; m_stressTestManager = nullptr;
    delete m_stressTestThread; m_stressTestThread = nullptr;

    emit enableUi(true);
    emit stressTestprogress("Measure canceled by user.", 0);
    m_isStressTesting = false;
    emit stressTesting(m_isStressTesting);
    emit finishedStressTesting();
}

Any ideas on how to fix this ? I used this pattern before (not exactly the same) on GNU/Linux and never had a problem with it. Is it a Qt bug ?

Update :
I have also this method that will be executed on the main thread :

void Application::onStressTestFinished(StressTestStatus testStatus)
{
	m_isStressTesting = false;
	emit stressTesting(m_isStressTesting);
	emit finishedStressTesting();
	emit enableUi(true);
	emit stressTestprogress("Ready for a new stress test", 100);

	if (testStatus == StressTestStatus::ErrorOccurred ||
		testStatus == StressTestStatus::StoppedByUser)
	{
		m_logger->log(LogLevel::info, LogSource::all, "Stress test aborted.");
	}

    if (m_stressTestThread == nullptr || m_stressTestManager == nullptr)
    {
        return;
    }

    m_stressTestThread->quit();
    m_stressTestThread->wait();

    delete m_stressTestManager; m_stressTestManager = nullptr;
    delete m_stressTestThread; m_stressTestThread = nullptr;
}

But I don't think it is executed since we are still blocked on "onStopStressTest".

My Qt version is 5.15.3 and is provided by M$ C++ library manager 'vcpkg'.

[Christian Ehrlicher]

You don't run an event loop so how should the slot be executed in your loop?

And what's all with this strange QMetaObject::invokeMethod() stuff? Why not use simple signal/slot connections? This is very unreadable ...

[embeddedmz_2]

You don't run an event loop so how should the slot be executed in your loop?

isCanceled() calls processEvents() which in turns calls processEvents on thread event dispatcher. I ran the method with a step by step execution.

And what's all with this strange QMetaObject::invokeMethod() stuff? Why not use simple signal/slot connections? This is very unreadable ...

For me it's fine, I don't want to create connections. And this is why I like Qt : this is message passing between threads which doesn't exist natively with C# or Java.

[embeddedmz_2]

Since I think it's a Qt bug, I will make a small example and share it with you.

[Christian Ehrlicher]

Also I don't see the setKeepStressTesting() slot in your class. Please simplify the code so it's readable and understandable so we can try to reproduce the problem.

[embeddedmz_2]

@Christian-Ehrlicher If I just call :

m_stressTestThread->quit();
m_stressTestThread->wait();

while startStressTest is still executing, the app freezes (e.g. the issue we are discussing, see the call stack above).

NB : In normal conditions, where these 2 instructions are called after startStressTest has finished (e.g. the user didn't abort the stress test), there's no issues.

When the app is freezing, the CAN bus thread is still working and emitting objects (so the main thread event loop list is accepting data, on VS2019, I can see the memory consumption increasing).

setKeepStressTesting :

void StressTestManager::setKeepStressTesting(const bool keep)
{
    if (m_Internals->KeepStressTesting != keep)
    {
        m_Internals->KeepStressTesting = keep;
    }
}

Maybe it's over-engineered but I prefer that threads communicate with each other using "messages" (it's zealous but when all your life you see buggy and crappy multithreaded code with race conditions and crashes all over the place, I think you can understand me). Anyway, the issue here isn't related to setKeepStressTesting. but maybe it is not a good idea to call the slot quit() when playing with the dispatcher I don't know, the doc says that quit() is thread safe. Monday, I will complete a demo program to show this bug on Windows.

[embeddedmz_2]

maybe dispatcher->processEvents(QEventLoop::AllEvents); restarts the event loop whereas is has already been stopped with a call to quit() ?

What do you think ?

[Christian Ehrlicher]

@embeddedmz_2 said in Infinite wait when stopping QThread event loop and then calling wait:

m_stressTestThread->quit();
m_stressTestThread->wait();

while startStressTest is still executing, the app freezes (e.g. the issue we are discussing, see the call stack above).

Correct since you run your own event handling and therefore have to somehow stop this event handling by yourself. E.g. by setting a variable and exiting the loop when this variable is set to true as you don with 'KeepStressTesting '.

Again: Simplify your code for your own sake and for our reproducibility - from my pov it's not maintainable/understandable with all those QMetaObject calls and the dozens of lambdas in one lengthly function.

[embeddedmz_2]

@Christian-Ehrlicher I made an example here (I extended another example I made to illustrate a Qt UI bug) : https://github.com/embeddedmz/QTableViewAdjustPolicyNotWorkingProperly build the program, run it, click on the Start button, then click on the Stop button, the application will freeze since the main thread is waiting for the stress test to finish but this last should be already finished because we had called quit() but it is still active (and no it is not blocked by my slot which ended after clicking on the Start button).

I don't know if I should fill a bug report. it would be nice if someone could confirm that the call to dispatcher->processEvents(QEventLoop::AllEvents) restarts the event loop and therefore cancels the effect of the quit() call.

In the meantime, I'll put a solution based on the QThread::finished signal.

[J.Hilk]

@embeddedmz_2
again, like @Christian-Ehrlicher said, its not a bug.

Do you know what quit() does ?

Tells the thread's event loop to exit with return code 0 (success). Equivalent to calling QThread::exit(0).

This function does nothing if the thread does not have an event loop.

Note: This function is thread-safe.

See also exit() and QEventLoop.

QThread will not interrupt any for/while loops that are currently running. It will quit, once it gets control again and instead of processing the event queue it will quit the execution.

You have to either listen in your worker for a quit condition, or use the discouraged terminate function, instead of quit.

[embeddedmz_2]

@J-Hilk said in Infinite wait when stopping QThread event loop and then calling wait:

It will quit, once it gets control again and instead of processing the event queue it will quit the execution.

but after startStressTest finished its work (and that's what happened when I clicked on the stop button), the event loop should have stopped but it didn't.

I'll repeat what I said in one of the previous messages: if you don't click on the stop button and let the slot finish its execution and after that you call quit and wait, there is no problem.

I think that the call to dispatcher->processEvents(QEventLoop::AllEvents) restarts the event loop and therefore cancels the effect of the quit() call.

Please, build and play with the example I provided you. Don't ignore the informations I have posted in my replies, look at the call stacks of the stress test thread for god's sake ! Thank you.

[embeddedmz_2]

To prove you I'm right, update MainWindow::stopStressTest and StressTestManager::startStressTest with this new code :

void MainWindow::stopStressTest()
{
    if (m_stressTestThread == nullptr || m_stressTestManager == nullptr)
    {
        return;
    }

    /*QMetaObject::invokeMethod(m_stressTestManager, "setKeepStressTesting",
        Qt::QueuedConnection, Q_ARG(bool, false));*/
    m_stressTestManager->setKeepStressTesting(false);

    m_stressTestThread->quit(); // QT bug : doesn't work since wait() will wait indefinitely
    m_stressTestThread->wait();

    delete m_stressTestManager; m_stressTestManager = nullptr;
    delete m_stressTestThread; m_stressTestThread = nullptr;

    m_isStressTesting = false;

    QMessageBox::warning(this, "Application", "Stress test aborted !");
}

void StressTestManager::startStressTest()
{
    for (size_t test = 0; test < 100 && /*!testError &&*/ /*!isCanceled()*/ m_Internals->KeepStressTesting; ++test)
    {
        QThread::msleep(100);
    }

    //processEvents();

    emit stressTestCompleted();
}

Now, there's no freeze because I don't call processEvents anymore on the dispatcher, try to uncomment isCanceled or processEvents and you will reproduce the bug.

[embeddedmz_2]

There's also something strange I have noticed with this code :

QObject::connect(m_canBusThread, &QThread::finished,
                     m_canBusManager, &CanBusManager::stop);

m_canBusManager->moveToThread(m_canBusThread);
    m_canBusThread->start();

the CanBusManager::stop will be executed in the thread managed by m_canBusThread but in the documentation this is what it says about the QThread::finished signal : When this signal is emitted, the event loop has already stopped running. But it's not true, you can check by yourself : put a breakpoint on the slot and check the thread/callstack : it is executed on the thread so the event loop is still running (or maybe something has restarted it. IDK someone needs to look to the source code).

[Christian Ehrlicher]

@embeddedmz_2 said in Infinite wait when stopping QThread event loop and then calling wait:

but after startStressTest finished its work (and that's what happened when I clicked on the stop button), the event loop should have stopped but it didn't.

Again: you don't run an event loop you just call processEvents() now and then. Simplify your code so we can reproduce it.

[embeddedmz_2]

@Christian-Ehrlicher I have posted an example here : https://github.com/embeddedmz/QTableViewAdjustPolicyNotWorkingProperly what's the problem ? If you don't take the time to read my answers completely, we can't move forward !

[Christian Ehrlicher]

Your example works fine for me. Add a debug output to StressTestManager::setKeepStressTesting() to see if you get there.

[embeddedmz_2]

The solution I found for now, is to call quit() at the end of the slot so that the main thread won't wait undefinitely. It is clearly a Qt bug (the QThread::finished signal is another one, see above).

QThread::currentThread()->quit();

[embeddedmz_2]

@Christian-Ehrlicher What operating system did you use ?

[Christian Ehrlicher]

It works fine for me by accident since it's a timing issue in your code.
You got the signal to stop the thread and handle it in setKeepStressTesting(). But before this is done, QThread::quit() was called and all active eventloops were notified about it and exited. But then you call processEvents() again and start the eventloop again.
Use QThread::interrupt() and QThread::isInterruptionRequested() since this sets an internal QThread variable which prevents the restart of the eventloop handling.

[embeddedmz_2]

@Christian-Ehrlicher I built the example on Ubuntu 20.04 LTS and there's absolutely no problem.

Comment out the line that invokes setKeepStressTesting, let the main thread call quit and wait on the QThread object, wait for the stress test thread to finish running gracefully and unlike on Windows 10, the wait() will return and everything is alright (no freeze).

I will file a bug report since the behavior is not the same on both platforms. The Windows port clearly has a problem.

As for the problem with the QThread::finished signal, the documentation is incorrect. The behavior is the same on both Windows and Ubuntu : the slot of the object belonging to a QThread is executed on the thread manged by that QThread. So the event loop is still running after that signal is fired (or maybe it is restarted and then shut properly since I don't see the thread in the list when I pause the program after the execution of the slot).