Is this multithreading code safe?

Crag_Hack

Hi I'd like to know if my multithreading code is safe (I'm no multithreading expert). Are there any races here between the setData function and starting the thread? Would it be better to use signals and slots to pass the backupJob list to the worker thread object or is my method good enough? Worker is a class derived from QObject that contains the worker thread code (just backing up files). The thread is created in program's QWidget main gui thread constructor. advancedCheckedBackup is called by clicking the backup button which then processes the backup jobs. The setupworker function just sets up the signals and slots for the worker object and thread. The jobList variable is a list of custom BackupJob objects that contain backup job information (source, destination, etc). It's pretty simple code. So I just have the two mentioned questions about this code (I have other questions but let's do this first...) Thanks!

void Replicator::advancedCheckedBackup()
{
    worker = new Worker;
    worker->moveToThread(workerThread);
    setupWorker(normal);

    QList<BackupJob> jobList;
    for (int backupCount = 0; backupCount < pendingQueue.size(); backupCount++)
    {
        jobList << advancedJobs[pendingQueue[backupCount]];
    }

    worker->setData(jobList);
    workerThread->start();
}

void Replicator::setupWorker(JobType jobType)
{
    if (jobType == normal)
    {
        connect(workerThread, SIGNAL(started()), worker, SLOT(backup()));
        connect(worker, SIGNAL(finished(bool)), this, SLOT(backupDone(bool)));
    }

    connect(worker, SIGNAL(finished(bool)), workerThread, SLOT(quit()));
    connect(worker, SIGNAL(finished(bool)), worker, SLOT(deleteLater()));
}

void Worker::setData(QList<BackupJob> jobs) { this->jobs = jobs; qDebug() << "setdata" << jobs[0].getName() << QThread::currentThreadId(); }

dheerendra

Once the backup button is clicked, Is the backup button disabled till the backup is complete ? If not you may have issue as user may keep clicking the button.

jsulm

@Crag_Hack Do you use same thread for all workers?
I mean this line:

worker->moveToThread(workerThread);

Do you create a new thread for each new worker or do you use same thread?

Crag_Hack

Thanks guys.
@dheerendra Yes that has been taken care of.
@jsulm That would be one of my other questions. Currently I reuse the same thread and I have connected these guys:

connect(worker, SIGNAL(finished(bool)), workerThread, SLOT(quit()));
connect(worker, SIGNAL(finished(bool)), worker, SLOT(deleteLater()));

So is it OK to reuse the same thread as long as I delete the worker object and quit the thread?
Also my initial concern is regarding the possibility of a race between the setData function and the starting of the thread. Gotta make sure the data gets passed before the thread starts. Is the code free of race conditions?

jsulm

@Crag_Hack It is probably OK as long as you make sure the previous worker finished its work, but I never tried this, so cannot tell for sure.

J.Hilk

@Crag_Hack
the others are right, your implementation is totaly fine. Except for the special case that when you try to start the thread anew while its still running.

To improve on it, I would suggest to pass jobList with SIGNAL/SLOT mechanics. That way you don't have to quit the thread when the worker is done. You simply pass it a new set of data.
Sig/Slot are, when used between different threads always queued -> While your worker ist still processing your previous dataset, the Signal with the new dataset will wait untill the function is finished.

Makes everything fuction a bit smoother :-)

PS: Even so you thread is not yet started, passing data between 2 threads like this worker->setData(jobList); is just bad manners :p

Crag_Hack

Thanks again! I have several more quick questions:

For signal/slot data passing from main gui thread to worker thread do I just connect a main gui thread signal to a slot in the worker object and emit the signal when the data copy needs to be done? Do I need to do qRegisterMetaType<QList< BackupJob>>();? Once the data is passed how do I start the thread?
The data copies are to be done on demand one job or joblist at a time so do I need to quit the thread when a copy completes? Will it be using resources if I don't quit it?
Do I need to exec() the thread for it to receive and send signals?

Less important:

Signal emission is thread safe by copying all passed data to the receiving slot right?
I have a cancel button linked to setting a cancel variable to true in the worker object. Is this an atomic operation? Should I be using std::atomic_bool?
Last question: for one of my features I have to pass a backupjob object from worker thread to main gui thread. backupJob objects consist only of reentrant data members. Will signal emission copy such an object in a thread-safe manner?

JKSH

@Crag_Hack said in Is this multithreading code safe?:

For signal/slot data passing from main gui thread to worker thread do I just connect a main gui thread signal to a slot in the worker object and emit the signal when the data copy needs to be done?

Yes.

Do I need to do qRegisterMetaType<QList< BackupJob>>();?

Most likely. Try it and see.

Once the data is passed how do I start the thread?

You need to start the thread (by calling QThread::start() before that thread can receive any signals.

The data copies are to be done on demand one job or joblist at a time so do I need to quit the thread when a copy completes? Will it be using resources if I don't quit it?

If it's only one thread, and if you will use it again soon, just leave the thread running. It won't use any detectable amount of resources while sleeping.

Do I need to exec() the thread for it to receive and send signals?

Assuming that you instantiated QThread without subclassing it, then QThread::start() already implicitly calls QThread::exec(). You don't need to call exec() directly.

If you're interested in the details, exec() runs the the thread's event loop. An event loop is required for the thread to receive signals (but it's not required to emit signals).

Signal emission is thread safe by copying all passed data to the receiving slot right?

Yes, if you use a QueuedConnection or an AutoConnection.

DirectConnection does not copy the data (but DirectConnection cannot be used for inter-thread communication, anyway)

I have a cancel button linked to setting a cancel variable to true in the worker object. Is this an atomic operation? Should I be using std::atomic_bool?

Yes, and yes.

Last question: for one of my features I have to pass a backupjob object from worker thread to main gui thread. backupJob objects consist only of reentrant data members. Will signal emission copy such an object in a thread-safe manner?

Yes.

Therefore, make sure that the cost of making this copy is less than the cost of running the job directly in your main thread.

Crag_Hack

One last quick question: how do I clean up after myself when closing the program with regard to the worker object and qthread?

JKSH

@Crag_Hack said in Is this multithreading code safe?:

One last quick question: how do I clean up after myself when closing the program with regard to the worker object and qthread?

Make sure you wait for the thread to finish before you close the program. See http://doc.qt.io/qt-5/qthread.html#wait

Crag_Hack

Do I need to do deletelater on the worker object and thread? If so do I block until deletelater succeeds?

JKSH

@Crag_Hack said in Is this multithreading code safe?:

Do I need to do deletelater on the worker object and thread?

You already call deleteLater() on your worker object when the task finishes, right?

If so do I block until deletelater succeeds?

Yes for the worker object, because it is deleted in another thread.

The QThread object lives in your main thread, so just treat it the same as any other QObject in your main thread. How do you delete those?

Crag_Hack

You already call deleteLater() on your worker object when the task finishes, right?

I did...but I decided to remodel after everything discussed in this thread and reuse the same object and thread instead. Multithreading in that sentence woah.... HA :)

The QThread object lives in your main thread, so just treat it the same as any other QObject in your main thread. How do you delete those?

Hmmm... using a simple delete I think. Never thought about individual QT class object's relationships to QObject though I surmise they all derive from such correct?

JKSH

@Crag_Hack said in Is this multithreading code safe?:

You already call deleteLater() on your worker object when the task finishes, right?

I did...but I decided to remodel after everything discussed in this thread and reuse the same object and thread instead. Multithreading in that sentence woah.... HA :)

OK. In that case, it would be easiest to delete your worker object right before you quit the worker thread (and your program). Do something like this in your main thread:

// Make worker->deleteLater() run in the worker thread, and block until the deletion is complete
QMetaObject::invokeMethod(worker, "deleteLater", Qt::BlockingQueuedConnection);

// ...
// At this point in your code, your worker is guaranteed to be gone.
// ...

// Stop the thread, and block until the thread is fully stopped
workerThread->quit();
workerThread->wait();

// ...
// At this point in your code, the worker thread is guaranteed to have stopped. You can now delete your QThread object which was managing that worker thread.
// ...

For more info, see:

The QThread object lives in your main thread, so just treat it the same as any other QObject in your main thread. How do you delete those?

Hmmm... using a simple delete I think. Never thought about individual QT class object's relationships to QObject though I surmise they all derive from such correct?

See http://doc.qt.io/qt-5/objecttrees.html

QObjects can form parent-child relationships. For most QObject-derived classes, the last parameter in their constuctor is for specifying the parent.

When QObject is deleted, its child objects are automatically deleted too. Conventionally, Qt developers often rely on this feature, so most QObjects don't need to be explicitly deleted. For example, make your QThread a child of your main window. The window gets deleted at shutdown, so it auto-deletes your QThread too.

deleteLater() ensures that all signals/events for an object are processed before the object is deleted. If you are 100% that your object has nothing left to process, you can use a simple delete. If you're not sure, use deleteLater() to be safe.

J.Hilk

@JKSH said in Is this multithreading code safe?:

When QObject is deleted, its child objects are automatically deleted too. Conventionally, Qt developers often rely on this feature, so most QObjects don't need to be explicitly deleted. For example, make your QThread a child of your main window. The window gets deleted at shutdown, so it auto-deletes your QThread too.

Hold on, I'm pretty sure, that if you make your thread a child of your main window, that you don't have a 100% seperated new thread. Something like this for example:

QThread * thread = new QThread(this);
worker *threadObject = new worker(this);
connect(thread, &QThread::started, threadObject, &worker::startWork);

threadObject->moveToThread(worker);
thread->start();

Both main thread and worker thread have the same currentThreadId()
and a big calculation should also freeze your main gui.

Edit: renamed threadObject.

kshegunov

Just connect the thread's finished() signal to the worker object's deleteLater() slot and you'll be fine, e.g:

QThread thread;
thread.start();

QObject * worker = new QObject();
QObject::connect(&thread, &QThread::finished, worker, &QObject::deleteLater);

// ... more code ...

There's no need to block the worker thread, you just need to wait for it to gracefully finish as @JKSH said. The deferred deletion events are guaranteed to be executed on thread exiting (look up the docs for the exact quotation).

kshegunov

@J.Hilk said in Is this multithreading code safe?:

Both main thread and worker thread have the same currentThreadId()
and a big calculation should also freeze your main gui.

Of course they have. The thread objects all live in the main thread in your code and that's absolutely normal. The QThread class manages a thread, it is not a thread itself. The only thing that's executed in a new thread context is QThread::run (which you don't touch when doing the worker object anyway). The slots of your worker object are executed in the new thread (granted you don't use Qt::DirectConnection when connecting).

J.Hilk

@kshegunov
thanks, that supports my point. I beleive ? :-)

Changing my sample to not having parents:

QThread * thread = new QThread();
worker *threadObject = new worker();
connect(thread, &QThread::started, threadObject, &worker::startWork);

threadObject->moveToThread(worker);
thread->start();

Separates them, but one has to handle the delete 'manualy' with

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

kshegunov

@J.Hilk said in Is this multithreading code safe?:

thanks, that supports my point. I beleive ? :-)

Which point you mean? I was responding to this:

a big calculation should also freeze your main gui.

which is certainly not correct. The big calculation you do in a slot in the worker object is executed in a separate thread, so it doesn't block the GUI thread.

Changing my sample to not having parents

Parents don't matter here, except for the moveToThread call - you can't move a child object to a thread different from the parent object's thread.

Separates them, but one has to handle the delete 'manualy' with

Surely C++ requires you to manage your memory, so "manually" handling deletions is not something new. I sometimes even create the worker object on the stack so the runtime frees it for me. E.g.:

int main(int argc, char ** argv)
{
    QCoreApplication app(argc, argv);

    QObject worker;
    QThread thread;

    worker.moveToThread(&thread);
    thread.start();

    // ... other stuff

    QMetaObject::invokeMethod(&app, "quit", Qt::QueuedConnection);
    QObject::connect(&app, &QCoreApplication::aboutToQuit, &thread, &QThread::quit);

    int ret = QCoreApplication::exec();

    thread.wait(); // ... and no `delete` is called at all, the stack unwinding will take care of the worker object.

    return ret;
}

JKSH

@J.Hilk said in Is this multithreading code safe?:

Hold on, I'm pretty sure, that if you make your thread a child of your main window, that you don't have a 100% seperated new thread.

Why?

Parent-child relationships only describe "ownership", and determines the order of automatically-deleted objects. These have absolutely no bearing on what threads get created in a program.

If a QThread instance is a child of a QMainWindow instance, that just means the QMainWindow "owns" the QThread instance, and will delete it.

Something like this for example:
QThread * thread = new QThread(this);
worker *threadObject = new worker(this);
connect(thread, &QThread::started, threadObject, &worker::startWork);

threadObject->moveToThread(worker);
thread->start();
Both main thread and worker thread have the same currentThreadId()
and a big calculation should also freeze your main gui.

QObject::currentThreadId() is a static function that tells you, "Which is the thread that called the currentThreadId() function?" It does not tell you "Which thread does a particular object live in?"

Add this to the end of your code snippet:

qDebug() << qApp->thread(); // Reports the thread affinity of your QApplication/QGuiApplication/QCoreApplication instance
qDebug() << thread->thread(); // Reprts the thread affinity of your QThread instance
qDebug() << threadObject->thread(); // Reports the thread affinity of your worker object instance

You'll find that threadObject lives in a different thread than qApp.

You'll also find that thread lives in the same thread as qApp, and that thread lives in a different thread than threadObject. (Yep, you read that right. Let this sink in slowly. A QThread is not a thread!)