QThread will not exit

Question

When I close my application, the thread is still running even though it should have ended. The following code will simply hang on the workerThread->wait(); line.

Main Thread

#include "mainwindow.h"
#include "ui_mainwindow.h"

MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::MainWindow)
{
    ui->setupUi(this);

    workerThread = new QThread();
    worker = new Worker();

    worker->moveToThread(workerThread);

    connect(this, SIGNAL(ThreadStopSignal()), worker, SLOT(ThreadStopSlot()));
    connect(this, SIGNAL(StartWorkerSignal()), worker, SLOT(RunSlot()));
    connect(worker, SIGNAL(finished()), workerThread, SLOT(quit()));
    connect(worker, SIGNAL(finished()), worker, SLOT(deleteLater()));
    connect(workerThread, SIGNAL(finished()), workerThread, SLOT(deleteLater()));

    workerThread->start();
    emit(StartWorkerSignal());
    qDebug()<<"Main thread: " << QThread::currentThreadId();
}

MainWindow::~MainWindow()
{
    qDebug() << "Asking threads to exit" << QThread::currentThreadId();
    emit(ThreadStopSignal());

    workerThread->wait();

    qDebug() << "thread is dead";
    delete ui;
}

Worker implementation

#include "worker.h"

#include <QCoreApplication>

Worker::Worker()
{
    allowRun = true;
}

void Worker::RunSlot()
{
    qDebug() << "Starting";
    int i = 0;
    while(allowRun)
    {
        QThread::msleep(1000);
        QCoreApplication::processEvents();
        qDebug() << i++ << QThread::currentThreadId();
    }
    emit finished();
    qDebug() << "Done counting";
}

void Worker::ThreadStopSlot()
{
    allowRun = false;
    qDebug() << "Ending HID WORKER" << QThread::currentThreadId();
}

A typical run will produce the following output (Qt 5.1.1 clang x86_64)*:

Main thread:  0x7fff743b2300 
Starting 
0 0x10b6e1000 
1 0x10b6e1000 
2 0x10b6e1000 
3 0x10b6e1000 
4 0x10b6e1000 
5 0x10b6e1000 
6 0x10b6e1000 
7 0x10b6e1000 
8 0x10b6e1000 
9 0x10b6e1000 
10 0x10b6e1000 
11 0x10b6e1000 
Asking threads to exit 0x7fff743b2300 
Ending HID WORKER 0x10b6e1000 
12 0x10b6e1000 
Done counting 

However, the application will still be running, just with no UI; it will occasionally crash, causing an Apple send-crash-report dialog to open.

*Yes, I'm currently stuck with a slightly older Qt version. However, I have tested this on newer builds and have achieved similar results.

Answer 1

The problem is rather simple: You block the event loop in the main thread when you wait for the thread to finish. Yet, at the same time, the event loop must receive the workerThread->quit() call. Thus you have a deadlock.

The simple fix is to explicitly quit() the thread before wait()ing for it. You achieve this by fixing the inherent brokenness of QThread. See below for a safe implementation of the Thread class. You can then simply destruct the thread in MainWindow's destructor.

Alas, the code is has some antipatterns and voodoo.

1. Neither workerThread nor worker should be pointers. This is premature pessimization since you add an extra heap allocation and an extra layer of indirection. It's pointless, and forces you to do manual memory management where none is called for.

2. The threadStopSlot is thread-safe and there's no reason to invoke it from the worker thread's event loop. You can call it directly. When you do so, you don't need to call QCoreApplication::processEvents in runSlot. When you do that, you re-enter the event loop, and suddenly all of your objects running in that thread are subject to reentrancy requirement and must be audited as such. Bad idea - don't do it.

3. Since you probably wish to let the event loop run in the worker's thread, you should invert the control: instead of keeping the control in runSlot, keep it in the event loop, and have the event loop call runSlot repeatedly. That's what zero-timeout timer idiom is for.

4. Initializer lists give rise to idiomatic C++. Use them.

5. emit is meant as a prefix, not a function. You emit fooSignal(), not emit(fooSignal()). It's a matter of style, true, but emit is for documentation purposes. It is only meant for human consumption, and we humans read things easier when they are not wrapped in an extra layer of parentheses. If you don't care for the documentation aspect, don't use emit at all. Signals are regular methods with machine-generated implementation. You don't need to call them in any special way.

6. Since you use Qt 5, you should be using compile-time-checked connect syntax. This does not require a C++11 compiler - not unless you use lambdas too.

7. It's probably a bad idea to ask the threads to quit in the destructor of a window, since you might have other things to do in the main thread as the worker threads quit - things that necessitate the event loop to be running.

   The only way a window would be destructed is if it has WA_DeleteOnClose attribute, or if you've quit the main event loop and are destructing the window as you exit main(). You should catch the window's close event and set things in motion such that the window gets deleted only when everything that should be done, is done.

8. Instead of outputting the thread id to qDebug(), you can output the thread itself. You can leverage the fact that threads are objects and you can give them human readable names. You then don't need to compare thread ids or addresses manually, you simply read the thread's name.

Given all the above, if you asked me to write the code, I'd do it as follows.

First, the worker functionality can be abstracted out into a worker base:

#include <QtWidgets>

class WorkerBase : public QObject {
   Q_OBJECT
   Q_PROPERTY(bool active READ isActive WRITE setActive)
   QBasicTimer m_runTimer;
   bool m_active;
protected:
   void timerEvent(QTimerEvent * ev) {
      if (ev->timerId() != m_runTimer.timerId()) return;
      work();
   }
   virtual void workStarted() {}
   virtual void work() = 0;
   virtual void workEnded() {}
public:
   WorkerBase(QObject * parent = 0) : QObject(parent), m_active(false) {
      setActive(true);
   }
   /// This method is thread-safe.
   bool isActive() const { return m_active; }
   /// This method is thread-safe.
   void setActive(bool active) {
      QObject source;
      QObject::connect(&source, &QObject::destroyed, this, [this,active]{
         // The functor is executed in the thread context of this object
         if (m_active == active) return;
         if (active) {
            m_runTimer.start(0, this);
            workStarted();
         } else {
            m_runTimer.stop();
            workEnded();
         }
         m_active = active;
      }, thread() ? Qt::QueuedConnection : Qt::DirectConnection);
   }
   ~WorkerBase() {
      Q_ASSERT(QThread::currentThread() == thread() || !thread());
      setActive(false);
   }
};

Then, the worker becomes straightforward:

class Worker : public WorkerBase {
   Q_OBJECT
   int m_runCount;
protected:
   void workStarted() Q_DECL_OVERRIDE {
      qDebug() << "Starting" << QThread::currentThread();
   }
   void work() Q_DECL_OVERRIDE {
      QThread::msleep(1000);
      ++ m_runCount;
      qDebug() << m_runCount << QThread::currentThread();
   }
   void workEnded() Q_DECL_OVERRIDE {
      qDebug() << "Finishing" << QThread::currentThread();
      emit finished();
   }
public:
   Worker(QObject * parent = 0) : WorkerBase(parent), m_runCount(0) {}
   Q_SIGNAL void finished();
};

Finally, we add the safe thread implementation, and a main window that keeps the locus of control in the event loop, and lets the loop run until everything is ready for the window to be destructed:

class Thread : public QThread {
   using QThread::run; // final method
public:
   Thread(QObject * parent = 0) : QThread(parent) {}
   ~Thread() { quit(); wait(); }
};

class MainWindow : public QMainWindow {
   Q_OBJECT
   Worker m_worker;
   Thread m_workerThread;
   QLabel m_label;
protected:
   void closeEvent(QCloseEvent * ev) {
      if (m_worker.isActive()) {
         m_worker.setActive(false);
         ev->ignore();
      } else
         ev->accept();
   }
public:
   MainWindow(QWidget * parent = 0) : QMainWindow(parent),
      m_label("Hello :)\nClose the window to quit.")
   {
      setCentralWidget(&m_label);
      m_workerThread.setObjectName("m_worker");
      m_worker.moveToThread(&m_workerThread);
      connect(&m_worker, &Worker::finished, this, &QWidget::close);
      m_workerThread.start();
      qDebug() << "Main thread:" << QThread::currentThread();
   }
   ~MainWindow() {
      qDebug() << __FUNCTION__ << QThread::currentThread();
   }
};

int main(int argc, char ** argv)
{
   QApplication a(argc, argv);
   QThread::currentThread()->setObjectName("main");
   MainWindow w;
   w.show();
   w.setAttribute(Qt::WA_QuitOnClose);
   return a.exec();
}

#include "main.moc"

Note the notable absence of the plethora of signals/slots that your approach necessitated. All you care for is when the worker object has finished, you can then close the window and rip all objects to shreds.

Note that the declaration order is important. C++'s semantics are not random in this respect: the order has meaning. The worker thread must be declared after the worker object, since they will be destructed in the reverse order. Thus, the thread is quit and destroyed first. At that point, m_worker->thread() == nullptr and you can destroy the worker from any thread, including the main thread. It'd be an error if the worker's thread had still existed - you'd then need to destroy the worker in its own thread.

Output:

Main thread: QThread(0x7fa59b501180, name = "main")
Starting QThread(0x7fff5e053af8, name = "m_worker")
1 QThread(0x7fff5e053af8, name = "m_worker")
2 QThread(0x7fff5e053af8, name = "m_worker")
3 QThread(0x7fff5e053af8, name = "m_worker")
Finishing QThread(0x7fff5e053af8, name = "m_worker")
~MainWindow QThread(0x7fa59b501180, name = "main")