Question about QThread object deletion

Kirill Gusarev

Hi smart guys! I have doubts about the correctness of following code:
[mainwindow.cpp]

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

    thread = new QThread(this);
    connect(this, SIGNAL(destroyed()),
            thread, SLOT(quit()));

    worker = new IanColemanSeleniumWorker();
    connect(this, SIGNAL(startWork(uint64_t)),
            worker, SLOT(run(uint64_t)));
    worker->moveToThread(thread);
    thread->start();
}

MainWindow::~MainWindow()
{
    delete ui;
}

I wrote this code according to the video guide.
I'm worried about the line "thread = new QThread(this);". Is "this" argument necessary here? Please pay attention to line "connect(this, SIGNAL(destroyed()), thread, SLOT(quit()));".
I want the thread to exit gracefully when closing the application. Should a thread have a parent?

Pl45m4

@Kirill-Gusarev said in Question about QThread object deletion:

I'm worried about the line "thread = new QThread(this);". Is "this" argument necessary here?

It's not wrong, but not needed,

Please pay attention to line "connect(this, SIGNAL(destroyed()), thread, SLOT(quit()));".
I want the thread to exit gracefully when closing the application. Should a thread have a parent?

since, when you want have a clean worker every time, you can do something like:

MainWindow::spawnWorker(uint64_t workerArg) 
{
   QThread *thread = new QThread();
   IanColemanSeleniumWorker *worker = new IanColemanSeleniumWorker();
   worker->moveToThread(thread);

   // maybe error handling
   // post error to MainWindow
   connect( worker, &IanColemanSeleniumWorker::error, this, &MainWindow::errorString);

   connect( this, &MainWindow::startWork, worker, &IanColemanSeleniumWorker::run);
   // emit a finished() signal in your worker, once you are done
   connect( worker, &IanColemanSeleniumWorker::finished, thread, &QThread::quit);
   connect( worker, &IanColemanSeleniumWorker::finished, worker, &IanColemanSeleniumWorker::deleteLater);
   connect( thread, &QThread::finished, thread, &QThread::deleteLater);

   thread->start();
   emit startWork(workerArg);
}

This will create a new thread and worker, let the worker do its job and then they are cleaned up properly without the need to being child of MainWindow / this.
Keep in mind not to allocate new objects in the workers c'tor, because the c'tor is called when the worker is still in MainWindow's main GUI thread.

Also using the above approach you might need to handle the destroyed signal, like you did in your code, to stop/interrupt running workers when you want to exit.

Kirill Gusarev

@Pl45m4 Your code is beautiful.
Большое спасибо!

Kirill Gusarev

@Pl45m4 what does "c'tor" mean?

Pl45m4

@Kirill-Gusarev said in Question about QThread object deletion:

what does "c'tor" mean?

constructor

If your IanColemanSeleniumWorker class has private members:

IanColemanSeleniumWorker::IanColemanSeleniumWorker(QObject *parent)
  : QObject{parent}
{
  // DONT do something like that here
  // allocate memory and assign data AFTER the object was moved
  // ideally init your members at the beginning of your run function
  m_workerData = new SomeDataClass();
  m_intptr = new int;
}

Better:

IanColemanSeleniumWorker::run(uint64_t int)
{
   m_data = new SomeDataClass();

   // do work
   // . . .
   // . . . 

   emit finished(); // send finished signal when work is done
}

because of these lines

   // ...
  IanColemanSeleniumWorker *worker = new IanColemanSeleniumWorker(); // worker and all its childs in main thread
   worker->moveToThread(thread); // worker moved
   // worker belongs to "thread" Thread from now on
   // ...

Kirill Gusarev

@Pl45m4 said in Question about QThread object deletion:

Keep in mind not to allocate new objects in the workers c'tor, because the c'tor is called when the worker is still in MainWindow's main GUI thread.

What will happen if I allocate new objects in the worker's constructor?

jsulm

@Kirill-Gusarev The explanation provided by @Pl45m4 answers your question: worker constructor is not running in the other thread, so everything allocated in the constructor is allocated in the thread where the worker is created.

SimonSchroeder

@Kirill-Gusarev said in Question about QThread object deletion:

What will happen if I allocate new objects in the worker's constructor?

This mostly relates to objects derived from QObject. QObject is thread affine which is why you need to use moveToThread(). As said before, the constructor is not yet run inside the new thread, but only the run() method. This rule does not necessarily apply for objects not derived from QObject, but it is certainly easier to follow the rule to not create any object inside the constructor. This also keeps all initialization in one place (i.e. the run() method).

kshegunov

@SimonSchroeder said in Question about QThread object deletion:

As said before, the constructor is not yet run inside the new thread, but only the run() method.

Just so we are very clear - because the run method is the new thread ...

Kirill Gusarev

@jsulm Why is that bad?

jsulm

@Kirill-Gusarev See @SimonSchroeder explanation and also read https://doc.qt.io/qt-6/threads-qobject.html