Solved Is this multithreading code safe?

[JKSH]

@kshegunov said in Is this multithreading code safe?:

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).

There's one thing I've been meaning to investigate, but I haven't gotten around to: Is the deferred deletion guaranteed to finish before QThread::wait() returns? In other words: If the worker object's destructor takes a long time to run, is there a risk of the main thread quitting before the worker object is fully deleted?

[J.Hilk]

@JKSH @kshegunov

well, I'm not resisten to learning, hopefully.
So thanks for the detailed exmplanation, I'll have to read everything up once more I believe.

Complicated enough topic as it is.

[kshegunov]

@JKSH said in Is this multithreading code safe?:

Is the deferred deletion guaranteed to finish before QThread::wait() returns?

Yes, it is. The deferred deletions are treated in a special way so they're processed just before the thread actually quits (which implies they're processed after the finished() signal is emitted).

In other words: If the worker object's destructor takes a long time to run, is there a risk of the main thread quitting before the worker object is fully deleted?

Not if you wait() on the thread. Not calling wait() is another kettle of fish altogether and introduces even more complications, although it may be necessary sometimes when managing multiple threads and you want to wait for them all.

[Crag_Hack]

Hi here's the final modified code (signal/slot data pass instead of direct via worker method call and also reuse of thread and worker object). Sorry for being such a perfectionst but I don't want to take any risks with anybody's data considering this is a backup program.

Worker is a class derived from QObject that contains the worker thread code (just backing up files). The thread and worker object are created in the 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).

Is this code safe?

When using signal slot communication between threads it relies on the copy constructor to create a copy of the object passed to another thread right? If so my BackupJob objects get copied as reentrant objects properly right? (they only have reentrant data members except for one pointer (not sure if it's reentrant) to IVssBackupComponents for volume shadow copy service - this pointer is only used nonconcurrently inside each program backup job and freed upon completion of the job)

Also one signal not in the below code is emitted by my worker thread and passes a single backupjob object back to the main thread - this is safe by the same reasoning as previously mentioned regarding the passing of backup jobs to the worker thread right?

I think this entire scenario of using threads to implement a non-gui-blocking worker thread for prolonged operations without concurrent data modification is an excellent candidate for a thread example in the thread documentation here http://doc.qt.io/qt-5/threads.html What do you think? Donno if you guys can influence such.

Thanks!

Replicator::Replicator(QWidget *parent) :
    QWidget(parent), wizard(0)
{
    workerThread = new QThread(this);
    workerThread->start();
    worker = new Worker;
    worker->moveToThread(workerThread);
    setupWorker();
}

void Replicator::setupWorker()
{
    connect(workerThread, SIGNAL(finished()), worker, SLOT(deleteLater()));

    connect(this, SIGNAL(backup(QList<BackupJob>)), worker, SLOT(backup(QList<BackupJob>)));
    connect(worker, SIGNAL(finished(bool)), this, SLOT(backupDone(bool)));
}

void Replicator::advancedCheckedBackup()
{
    pendingQueue = mainScreen->getJobList();
    QList<BackupJob> jobList;
    for (int backupCount = 0; backupCount < pendingQueue.size(); backupCount++)
    {
        jobList << advancedJobs[pendingQueue[backupCount]];
    }

    emit backup(jobList);
}

void Worker::backup(QList<BackupJob> jobs)
{
    this->jobs = jobs;

	***backup code here***
}

Replicator::~Replicator()
{
    workerThread->quit();
    workerThread->wait();
}

[kshegunov]

@Crag_Hack said in Is this multithreading code safe?:

Is this code safe?

It looks okay to me.

When using signal slot communication between threads it relies on the copy constructor to create a copy of the object passed to another thread right?

Yes.

If so my BackupJob objects get copied as reentrant objects properly right?

Yes.

they only have reentrant data members except for one pointer (not sure if it's reentrant) to IVssBackupComponents for volume shadow copy service

The pointer is reentrant. Whether the object it is pointing to is thread-safe however is another matter altogether.

this pointer is only used nonconcurrently inside each program backup job and freed upon completion of the job.

What about the object the pointer holds a reference to? The point is the pointer may very well be reentrant, but the object it points to might not be thread safe, which would imply you can't call functions of that object in a thread-safe manner.

Also one signal not in the below code is emitted by my worker thread and passes a single backupjob object back to the main thread - this is safe by the same reasoning as previously mentioned regarding the passing of backup jobs to the worker thread right?

Yes.

Donno if you guys can influence such.

Anyone, including you, can write an example and submit the source code for review; that's the point of Qt being offered as open-source (under LGPL) as well as the commercial licensing. :)

[Crag_Hack]

The pointer is reentrant. Whether the object it is pointing to is thread-safe however is another matter altogether.

The pointer is one of these guys used for Volume Shadow Copy Service. Prior to control passing to the worker object the pointer is uninitialized not even null, during the work done by the worker object in the other thread the pointer is initialized through CreateVssBackupComponents, then it goes through a lengthy setup process to set up the volume shadow copy service options, then when the worker no longer needs it it is released through IVssBackupComponents::Release. It never passes from gui thread to worker thread or from worker thread to gui thread.

So it's completely isolated to each job dispatched to the worker object. Sounds safe right?

Thanks so much!