How can I have QThread emit a heap-allocated QObject without leaking?

Question

My situation is that I have a QWidget-derived class, MyWidget, that will create a QThread-derived class (WorkerThread) to do some uninterruptible, blocking work in its run() method. The results of this are a heap-allocated instance of a QObject-derived class (DataClass) which is then received and processed by MyWidget. MyWidget is a transitory widget, though, and may be deleted while WorkerThread is still running due to user action.

Here's some pseudo-code to illustrate this:

#include <QThread>
#include <QWidget>

class DataClass : public QObject {
    Q_OBJECT
public:
    // contains some complex data
};

class WorkerThread : public QThread {
    Q_OBJECT
public:
    virtual void run() {
        DataClass *result = new DataClass;
        doSomeReallyLongUninterruptibleWork(result);
        emit workComplete(result);
    }
signals:
    void workComplete(DataClass *);
};

class MyWidget : public QWidget {
    Q_OBJECT
public:
    void doBlockingWork() {
        WorkerThread *worker = new WorkerThread;
        connect(worker, &WorkerThread::finished, worker, &WorkerThread::deleteLater);
        connect(worker, &WorkerThread::workComplete, this, &MyWidget::processData);
        worker->start();
    }

public slots:
    void processData(DataClass *result) {
        // Do some stuff
        delete result;
        // Assuming MyWidget still exists when WorkerThread has finished, no memory has leaked
    }
};

Normally the correct "Qt" way to return the results of a worker thread is to have it emit a signal with its arguments being the result of its work, as illustrated above. That's fine for data that can be copied, but since the result is a pointer to a heap-allocated object, I have to be careful to make sure that memory gets freed.

And normally that wouldn't be a problem, because since WorkerThread has finished, I can safely pass the pointer to DataClass to MyWidget, have it process DataClass, and then free it.

The problem is that, as I said earlier, MyWidget is transitory and may be destroyed before WorkerThread is finishing. In this scenario, how can I ensure that the instance of DataClass gets freed one way or the other?

In particular, I'm looking for solutions that have some elegance to them, meaning that it takes advantage of Qt's features and preferably makes it so that WorkerThread maintains its separation from MyWidget so that WorkerThread doesn't need to know anything about it or any other class that might create it. I'm also open to ideas that improve upon the pattern that I'm already using.

Answer 1

Use smart pointer (e.g., QSharedPointer) instead a normal pointer:

DataClass *result = new DataClass;

should be replaced with

QSharedPointer<DataClass> result = QSharedPointer<DataClass>(new DataClass);

Then, you could safely pass it somewhere and do not worry about deleting it. When it is out of the last scope where it can be used, the object will be automatically destroyed.

Answer 2

The worker should push the result to the main thread, to indicate that it's safe to use there (per QObject semantics). The result should be auto-deleted in the main thread after everyone interested has been notified of the completion of the work. It is a minimal change:

void run() override {
   auto result = new DataClass;
   doSomeReallyLongUninterruptibleWork(result);
   result->moveToThread(qApp->thread());   // added
   emit workComplete(result);
   QObject::connect(this, &QThread::finished, result, &QObject::deleteLater); // added
}

You're guaranteed that deleteLater will be invoked after the last handler of workComplete has finished in the main thread.

A single object in the main thread might wish to retain the results longer. This can be indicated by setting the parent on the result object. The object shouldn't be deleted then:

...
QObject::connect(this, &QThread::finished, result, [result]{
  if (!result->parent()) result->deleteLater();
});

If you intend that multiple objects in the main thread retain the results longer, you should be using a QSharedPointer in the workComplete's argument, and you must never set the parent of the results: a non-null parent and a QSharedPointer are mutually incompatible: the former indicates a unique ownership by a parent, the latter indicates a shared ownership.

It is necessary to move the DataClass object to the main thread to avoid a race on DataClass::thead() and to allow deleteLater to work:

1. Worker Thread: emit workComplete(result)
2. Main Thread: start using result, result.thread() is the worker instance.
3. Worker Thread: finishes
4. Main Thread: result.thread() is now nullptr while the main thread is using it.

This might not be a problem, but usually indicates poor design. As soon as you start using more QObject features of DataClass, it turns the latent bug into a real bug: e.g. deleteLater won't work, timers won't work, etc.

Furthermore, destructing a QObject in any thread other than its thread is not supported. Suppose that you had your original code. The following could happen and leads to undefined behavior:

1. Worker Thread: emit workComplete(result)
2. Main Thread: start using result, result.thread() is the worker instance.
3. Main Thread: delete result. QObject::~QObject is invoked in qApp->thread() but result->thread() is the different, still live instance of the worker thread.

If you wish to catch such issues, add:

DataClass::~DataClass() {
  Q_ASSERT(thread() == nullptr || thread() == QThread::currentThread());
  ...
}

It's OK to destruct a threadless object, but such objects are not fully functional: you can't deleteLater them, their timers don't work, they don't receive events, etc.

The necessity of a parent check prior to deleteLater depends on whether you intend to prolong the existence of the result past the code connected to workComplete.

The "obvious" use of a shared pointer doesn't make it clear which thread can safely access the result iff the result isn't thread-safe. It also does nothing by itself to fix the fact that once the worker finishes, the QObject is half-functional as there's no event loop associated with it. I believe that your intent is that only one thread may own the result, so that its methods don't have to be thread-safe. Luckily, QObject's semantics already express this clearly: the object's thread() is the one authorized to act on the object.

Any recipients of workComplete in the main thread will get to process the results before they vanish. If any object in the main thread wants to take ownership of the result, it can - by setting the parent. Otherwise, as soon the workComplete handlers are done, if none have claimed ownership, the result will get deleted from the main event loop.

Change the QTimer::singleShot(1000, w.data(), [&]{ w.reset(); }) timer to 2500ms to have the widget outlive the worker thread and note the difference in behavior depending on whether it claimed ownership.

Complete example:

// https://github.com/KubaO/stackoverflown/tree/master/questions/worker-shared-37956073
#include <QtCore>

struct DataClass : public QObject {
   DataClass() { qDebug() << __FUNCTION__; }
   ~DataClass() { qDebug() << __FUNCTION__; }
};
void doSomeReallyLongUninterruptibleWork(DataClass*) { QThread::sleep(2); }

class WorkerThread : public QThread {
   Q_OBJECT
public:
   void run() override {
      auto result = new DataClass;
      doSomeReallyLongUninterruptibleWork(result);
      result->moveToThread(qApp->thread());
      emit workComplete(result);
      QObject::connect(this, &QThread::finished, result, [result]{
         if (!result->parent()) {
            qDebug() << "DataClass is unclaimed and will deleteLater";
            result->deleteLater();
         }
      });
   }
   Q_SIGNAL void workComplete(DataClass*);
};

class MyWidget : public QObject {
   void processData(DataClass * result) {
      // Do stuff with result
      // Retain ownership (optional)
      if (true) result->setParent(this);
   }
public:
   void doBlockingWork() {
      auto worker = new WorkerThread;
      connect(worker, &WorkerThread::workComplete, this, &MyWidget::processData);
      connect(worker, &WorkerThread::finished, worker, &WorkerThread::deleteLater);
      worker->start();
   }
   ~MyWidget() { qDebug() << __FUNCTION__; }
};

int main(int argc, char ** argv) {
   QCoreApplication app{argc, argv};
   QScopedPointer<MyWidget> w{new MyWidget};
   w->doBlockingWork();
   QTimer::singleShot(1000, w.data(), [&]{ w.reset(); });
   QTimer::singleShot(3000, qApp, &QCoreApplication::quit);
   return app.exec();
}

#include "main.moc"

You could also forgo the use of an explicit thread, and use QtConcurrent::run instead. There's no clear advantage to that, I'm showing it here just to indicate that either approach is feasible.

#include <QtConcurrent>

struct DataClass : public QObject {
   Q_SIGNAL void ready();
   Q_OBJECT
};

// Let's not pollute the default pool with long-running stuff
Q_GLOBAL_STATIC(QThreadPool, longPool)

class MyWidget : public QObject {
   void processData(DataClass * result) {
      // Do stuff with result
      // Retain ownership (optional)
      if (true) result->setParent(this);
   }
public:
   void doBlockingWork() {
      auto result = new DataClass;
      connect(result, &DataClass::ready, this, [=]{ MyWidget::processData(result); });
      result->moveToThread(nullptr);
      QtConcurrent::run(longPool, [result]{
         result->moveToThread(QThread::currentThread());
         doSomeReallyLongUninterruptibleWork(result);
         result->moveToThread(qApp->thread());
         emit result->ready();
         QTimer::singleShot(0, result, [result]{
            if (!result->parent()) result->deleteLater();
         });
      });
   }
};