Safest way to delete workers when TCP server is closing

dream_captain

For now, in server destructor i iterate over active workers and explicitly delete them:

// for each worker call foo(...)
void foo (Worker *worker, QThread *thread)  {
    thread->quit();
    thread->wait(1000);
    delete worker
    delete thread;
}

Obviously, it's not the safest way to delete, because QThread can still be running.

But how about scheduled deletion? Is it ok to call deleteLater() right before the application will be closed (and event loop too).

J.Hilk

@dream_captain I would suggest doing it with Signal/Slots

e.g:

//Assuming:
QThread *myThread = new QThread();
Worker *myWorker = new Worker();

//Signal Slots:
connect(myThread , &QThread::finished,  myThread , &QThread::deleteLater);
connect(myThread , &QThread::finished,  myWorker , &Worker ::deleteLater);
connect(qApp, &QApplication::aboutToQuit,   myThread , &QThread::quit);

VRonin

@dream_captain said in Safest way to delete workers when TCP server is closing:

// for each worker call foo(...)
void foo (Worker *worker, QThread *thread)  {
    thread->quit(); //ok
    thread->wait(1000); // almost, use wait()
    delete worker; //NO
    delete thread; //NO
}

Just connect the thread finished signal to the deleteLater slots of the worker and thread as suggested above. it's safe (assuming you called QThread::exec())

dream_captain

@J.Hilk @VRonin
The code:

// for each worker call foo(...)
void foo (Worker *worker, QThread *thread)  {
    connect(thread, &QThread::finished, workerClient, &Worker::deleteLater);
    connect(thread,&QThread::finished,thread,&QThread::deleteLater);
    thread->quit();
}

results in error: QThread: Destroyed while thread is still running. Seems like i have to handle application close event as @J-Hilk suggested.

EDIT: Sorry, the error comes from another thread (my tcp server lives in his own thread). This code seems to work fine.

EDIT2: I've done some testing. It's strange, but destructor of QThread is not called in contrast or worker's destructor when the program is closing.

VRonin

the connects should be done when thread and workerClient are created
you forgot the "almost" part.

dream_captain

@VRonin
I made a small example illustrating the problem:

threadex.h

#ifndef THREADEX_H
#define THREADEX_H

#include <QThread>
#include <QDebug>

class ThreadEx : public QThread
{
    Q_OBJECT
public:
    explicit ThreadEx(QObject *parent = nullptr) :
        QThread(parent)
    {
        qDebug() << "thread: " << this << "created in: " << QThread::currentThread();
    }
    ~ThreadEx()
    {
        qDebug() << "thread: " << this << "deleted in: " << QThread::currentThread();
    }

};
#endif // THREADEX_H

main.cpp

#include <QApplication>
#include <QWidget>

#include "threadex.h"

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    ThreadEx *thread = new ThreadEx;
    QObject::connect(thread, &ThreadEx::finished, thread, &ThreadEx::deleteLater);
    QObject::connect(qApp, &QApplication::aboutToQuit,   thread, &ThreadEx::quit);
    QObject::connect(qApp, &QApplication::aboutToQuit,   []() {
        qDebug() <<  "aboutToQuit()";
    });
    thread->wait();  // don't know why i should block here, but this always returns true

    QWidget  widget;
    widget.show();

    return a.exec();
}

Output:

thread:  ThreadEx(0x768ca0) created in:  QThread(0x605fe0)
aboutToQuit()

ThreadEx::~ThreadEx() is not getting called. How can i be sure that cleanup was successfull and there is no running thread left?

Just for clarification:
QObject::deleteLater() documentation says that The object will be deleted when control returns to the event loop. Does that mean that deletion of all QObjects via deleteLater() is controlled by QApplication main event loop?

jsulm

@dream_captain said in Safest way to delete workers when TCP server is closing:

thread->wait();

why do you call wait?

dream_captain

@jsulm
To be sure that the thread has finished. I think that wait() is not necessary when we schedule deletion of QThread.

J.Hilk

@dream_captain
Good Morning,
wait will always return true, if the thread is not started, and in your example, the thread is not started. you simply call wait on it.

To expand your basic example:

#include <QApplication>
#include <QWidget>

#include "threadex.h"

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    ThreadEx *thread = new ThreadEx;
    QObject::connect(thread, &ThreadEx::finished, thread, &ThreadEx::deleteLater);
    QObject::connect(qApp, &QApplication::aboutToQuit,   thread, &ThreadEx::quit);
    QObject::connect(qApp, &QApplication::aboutToQuit,   []() {
        qDebug() <<  "aboutToQuit()";
    });
    thread.start();

....
....

    thread->quit();
    thread->wait();  
    
    QWidget  widget;
    widget.show();

    return a.exec();
}

dream_captain

@J.Hilk
Oh, I admit that i simply forget to start the thread. But shouldn't the deletion of thread be handled by QApplication in the snippet below? If i've understood the documentation correctly, there is no need to call quit() and wait() explicitly.


int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    ThreadEx *thread = new ThreadEx;

    QObject::connect(thread, &ThreadEx::finished, thread, &ThreadEx::deleteLater);
    QObject::connect(qApp, &QApplication::aboutToQuit,   thread, &ThreadEx::quit);
    QObject::connect(qApp, &QApplication::aboutToQuit,   []() {
        qDebug() <<  "aboutToQuit()";
    });
    thread->start();

    QWidget  widget;
    widget.show();

    return a.exec();
}

Still no ThreadEx destructor.

J.Hilk

@dream_captain Well, I'm not entierly sure,

I took inspiration from the example in the docs:

class Worker : public QObject
{
    Q_OBJECT
    QThread workerThread;

public slots:
    void doWork(const QString &parameter) {
        // ...
        emit resultReady(result);
    }

signals:
    void resultReady(const QString &result);
};

class Controller : public QObject
{
    Q_OBJECT
    QThread workerThread;
public:
    Controller() {
        Worker *worker = new Worker;
        worker->moveToThread(&workerThread);
        connect(&workerThread, SIGNAL(finished()), worker, SLOT(deleteLater()));
        connect(this, SIGNAL(operate(QString)), worker, SLOT(doWork(QString)));
        connect(worker, SIGNAL(resultReady(QString)), this, SLOT(handleResults(QString)));
        workerThread.start();
    }
    ~Controller() {
        workerThread.quit();
        workerThread.wait();
    }
public slots:
    void handleResults(const QString &);
signals:
    void operate(const QString &);
};

dream_captain

Updated example with Worker object.

worker.h

#ifndef WORKER_H
#define WORKER_H
#include <QObject>
#include <QDebug>
#include <QThread>
class Worker : public QObject
{
    Q_OBJECT
public:
    explicit Worker(QObject *parent = nullptr) :
        QObject(parent)
    {
        qDebug() << "worker: " << this << "created in: " << QThread::currentThread();
    }

    ~Worker()
    {
        qDebug() << "worker: " << this << "deleted in: " << QThread::currentThread();
    }
};
#endif // WORKER_H

threadex.h

#ifndef THREADEX_H
#define THREADEX_H
#include <QThread>
#include <QDebug>
class ThreadEx : public QThread
{
    Q_OBJECT
public:
    explicit ThreadEx(QObject *parent = nullptr) :
        QThread(parent)
    {
        qDebug() << "thread: " << this << "created in: " << QThread::currentThread();
    }
    ~ThreadEx()
    {
        qDebug() << "thread: " << this << "deleted in: " << QThread::currentThread();
    }
};
#endif // THREADEX_H

main.cpp

#include <QApplication>
#include <QWidget>

#include "threadex.h"
#include "worker.h"

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    ThreadEx *thread = new ThreadEx;

    Worker *worker = new Worker;

    worker->moveToThread(thread);

    QObject::connect(thread, &ThreadEx::finished, thread, &ThreadEx::deleteLater);
    QObject::connect(thread, &ThreadEx::finished, worker, &Worker::deleteLater);


    QObject::connect(qApp, &QApplication::aboutToQuit,   thread, &ThreadEx::quit);
    QObject::connect(qApp, &QApplication::aboutToQuit,   []() {
        qDebug() <<  "aboutToQuit()";
    });
    thread->start();


    QWidget  widget;
    widget.show();

    return a.exec();
}

Output:

thread:  ThreadEx(0x748d90) created in:  QThread(0x606fe0)
worker:  Worker(0x733020) created in:  QThread(0x606fe0)
aboutToQuit()
worker:  Worker(0x733020) deleted in:  ThreadEx(0x748d90)

Looks like QApplication process Worker's deleteLater() fine, but don't want to process ThreadEx's deleteLater().

jsulm

@dream_captain said in Safest way to delete workers when TCP server is closing:

But shouldn't the deletion of thread be handled by QApplication in the snippet below?

If you do not start the thread finished() signal will not be emitted and deleteLater() slot will not be called.
So, if you do not delete the thread explicitly using "delete" it will not be deleted.
Who and why should delete it in this case - it does not even have a parent? QApplication does not do memory management.

dream_captain

@jsulm
Yeah, i understand (just forget to start thread). In that example thread->start() was called though.
In the last example deletion of ThreadEx object should be handled by application main loop.