c++ - How to safely destruct a QThread?

Question

I want to properly destruct a QThread in Qt 5.3.

So far I have got:

MyClass::MyClass(QObject *parent) : QObject(parent) {
    mThread = new QThread(this);
    QObject::connect(mThread, SIGNAL(finished()), mThread, SLOT(deleteLater()));
    mWorker = new Worker(); // inherits from QObject
    mWorker->moveToThread(mThread);
    mThread->start();
}

MyClass::~MyClass() {
    mThread->requestInterruption();
}

My problem is that at the end of the day I still get:

QThread: Destroyed while thread is still running

Answer 1

The Safe Thread

In C++, the proper design of a class is such that the instance can be safely destroyed at any time. Almost all Qt classes act that way, but QThread doesn't.

Here's the class you should be using instead:

// Thread.hpp
#include <QThread>
public Thread : class QThread {
  Q_OBJECT
  using QThread::run; // This is a final class
public:
  Thread(QObject * parent = 0);
  ~Thread();
}

// Thread.cpp
#include "Thread.h"
Thread::Thread(QObject * parent): QThread(parent)
{}

Thread::~Thread() {
  quit();
  #if QT_VERSION >= QT_VERSION_CHECK(5,2,0)
  requestInterruption();
  #endif
  wait();
}

It will behave appropriately.

QObject Members Don't Need to Be on the Heap

Another problem is that the Worker object will be leaked. Instead of putting all of those objects on the heap, simply make them members of MyClass or its PIMPL.

The order of member declarations is important, since the members will be destructed in the reverse order of declaration. Thus, the destructor of MyClass will, invoke, in order:

1. m_workerThread.~Thread() At this point the thread is finished and gone, and m_worker.thread() == 0.

2. m_worker.~Worker Since the object is threadless, it's safe to destroy it in any thread.

3. ~QObject

Thus, with the worker and its thread as members of MyClass:

class MyClass : public QObject {
  Q_OBJECT
  Worker m_worker;          // **NOT** a pointer to Worker!
  Thread m_workerThread;    // **NOT** a pointer to Thread!
public:
  MyClass(QObject *parent = 0) : QObject(parent),
  // The m_worker **can't** have a parent since we move it to another thread.
  // The m_workerThread **must** have a parent. MyClass can be moved to another
  // thread at any time.
    m_workerThread(this)
  {
    m_worker.moveToThread(&m_workerThread);
    m_workerThread.start();
  }
};

And, if you don't want the implementation being in the interface, the same using PIMPL

// MyClass.hpp
#include <QObject>
class MyClassPrivate;
class MyClass : public QObject {
  Q_OBJECT
  Q_DECLARE_PRIVATE(MyClass)
  QScopedPointer<MyClass> const d_ptr;
public:
  MyClass(QObject * parent = 0);
  ~MyClass(); // required!
}

// MyClass.cpp
#include "MyClass.h"
#include "Thread.h"

class MyClassPrivate {
public:
  Worker worker;          // **NOT** a pointer to Worker!
  Thread workerThread;    // **NOT** a pointer to Thread!
  MyClassPrivate(QObject * parent);
};

MyClassPrivate(QObject * parent) :
  // The worker **can't** have a parent since we move it to another thread.
  // The workerThread **must** have a parent. MyClass can be moved to another
  // thread at any time.
    workerThread(parent)
{}

MyClass::MyClass(QObject * parent) : QObject(parent),
  d_ptr(new MyClassPrivate(this))
{
  Q_D(MyClass);
  d->worker.moveToThread(&d->workerThread);
  d->workerThread.start();
}

MyClass::~MyClass()
{}

QObject Member Parentage

We now see a hard rule emerge as to the parentage of any QObject members. There are only two cases:

1. If a QObject member is not moved to another thread from within the class, it must be a descendant of the class.

2. Otherwise, we must move the QObject member to another thread. The order of member declarations must be such that the thread is to be destroyed before the object. If is invalid to destruct an object that resides in another thread.

It is only safe to destruct a QObject if the following assertion holds:

Q_ASSERT(!object->thread() || object->thread() == QThread::currentThread())

An object whose thread has been destructed becomes threadless, and !object->thread() holds.

One might argue that we don't "intend" our class to be moved to another thread. If so, then obviously our object is not a QObject anymore, since a QObject has the moveToThread method and can be moved at any time. If a class doesn't obey the Liskov's substitution principle to its base class, it is an error to claim public inheritance from the base class. Thus, if our class publicly inherits from QObject, it must allow itself to be moved to any other thread at any time.

The QWidget is a bit of an outlier in this respect. At the very minimum, it should have made the moveToThread a protected method.

For example:

class Worker : public QObject {
  Q_OBJECT
  QTimer m_timer;
  QList<QFile*> m_files;
  ...
public:
  Worker(QObject * parent = 0);
  Q_SLOT bool processFile(const QString &);
};

Worker::Worker(QObject * parent) : QObject(parent),
  m_timer(this)  // the timer is our child
  // If m_timer wasn't our child, `worker.moveToThread` after construction
  // would cause the timer to fail.
{}

bool Worker::processFile(const QString & fn) {
  QScopedPointer<QFile> file(new QFile(fn, this));
  // If the file wasn't our child, `moveToThread` after `processFile` would
  // cause the file to "fail".
  if (! file->open(QIODevice::ReadOnly)) return false;      
  m_files << file.take();
}