Issue with QThread subclass and signal/slots

Joel Bodenmann

I'm currently working on a small tool which allows to run a regex search on all files within a directory (of course with tons of options).
My architecture mainly consists of the following classes:

Dialog (inherits QWidget)
Controller (inherits QThread)
Indexer (inherits QThread)
Worker (inherits QThread)

There's a button in the Dialog that allows launching a search performed by the Controller. The reason that Controller is running in a separate thread is to keep the GUI responsive.
Once the controller is asked to do it's job (by Dialog calling Controller::find()) it will launch an Indexer thread. The Indexer goes through the file system and emits the newFile() signal for each file that matches the search criteria and should therefore be searched by running the regex on it.
The Controller connects to the Indexer::newFile() signal and launches one Worker for each time the slot is being executed which results in having one worker per file. There's a Worker pool that Controller manages to be able to set some upper limit for the number of Workers running in parallel.

Here's all relevant code pasted:

If I run this (by calling Controller::find() from the GUI) I can see that the Indexer finishes very quickly (I get "Indexer::run(): finished" printed in the application output immediately) but the corresponding Controller::indexerFinished() slot is only invoked after a couple of seconds.
The GUI itself is supposed to display the results. Hence I forward the resultReady() signal of each Worker to the corresponding Controllers signal which the GUI listens too. I can observe the same behavior there: While I can confirm that the Workers are emitting the resultReady signal immediately but the GUI only receives them after/when the Controller::indexerFinished() slot is being executed.

Did I screw something up with the reimplementation of QThread::run() in the Controller class? I didn't call exec() in the Indexer::run() and Worker::run() methods as I don't need an event loop inside those threads.
I'd appreciate any help on this.

SGaist

Hi,

I don't see anything wrong with the implementation. Did you try to benchmark these classes ?

By the way, why not use QtConcurrent ? Looks like a good use case got it here.

kshegunov

I saw a couple of race conditions. :)
Now I want to be sure of something before we continue on:
You do realize that every slot of the QThread subclass is executed in the thread the QThread object lives in, correct?
This would include FindController::indexerFinished, FindController::processFile, FindController::resultReady and so on.

In that same spirit, connect(worker, &FindWorker::finished, [=]{ workerFinished(worker); }); will make your workerFinished function be executed in the worker's thread (FindWorker class), which is a race condition as you touch the common data from that method.

And a minor one that I spotted:

void FindWorker::run()
{
    // Open the file
    do {
    // ...
    } while (!line.isNull() && !_stopRequested); //< Reading `_stopRequested` while not protected
}

void FindWorker::requestStop()
{
    static QMutex mutex; //< Takes care of write protection, but not of reading it in another context. :)
 
    mutex.lock();
    _stopRequested = true;
    mutex.unlock();
}

For that thing you ought to use requestInterruption() and isInterruptionRequested() from QThread instead.

SGaist

@kshegunov Nice catches ! I did miss some interesting cases...

If you insist on keeping _stopRequested , you should make it an std::atomic otherwise, you might get surprising optimisation depending on how you access it.

Joel Bodenmann

@kshegunov said in Issue with QThread subclass and signal/slots:

You do realize that every slot of the QThread subclass is executed in the thread the QThread object lives in, correct?

Nope, I don't. Or at least, I didn't until know.
I thought that was only the case if Qt::DirectConnection is used. The way I understand the documentation, the slot should be executed in the thread that "owns the method" if Qt::QueuedConnection is used, no? What am I missing?

However, I can confirm that that's what's happening. That's why the GUI is blocked: I managed to prove that all of the heavy code in FindController is being executed in the GUI thread.

Regarding the mutex stuff: I assume (and that might be the problem) that reading a boolen is an atomic operation anyway. At least that's how it works in my world (embedded stuff). Is this different here? Am I missing something completely obvious?
Anyway, I'm definitely checking out QThread::requestInterruption() and QThread::isInterruptionRequested(). That seems like a decent choice :p

@SGaist: I don't insist on anything but doing everything the right way. So if there's a better way (which there seems to be) I'm certainly going to choose that one.

kshegunov

@SGaist said in Issue with QThread subclass and signal/slots:

Nice catches !

Thanks! :D

If you insist on keeping _stopRequested , you should make it an std::atomic otherwise, you might get surprising optimisation depending on how you access it.

This is what requestInterruption does internally, so I do advice making use of the already provided API. :)

Joel Bodenmann

@Joel-Bodenmann said:

Regarding the mutex stuff: I assume (and that might be the problem) that reading a boolen is an atomic operation anyway. At least that's how it works in my world (embedded stuff). Is this different here? Am I missing something completely obvious?

Edit: Thinking about this I just realized that I'm dealing with a multi-core system here, so things "are different" than on my beloved microcontrollers :p

VRonin

@Joel-Bodenmann said in Issue with QThread subclass and signal/slots:

The way I understand the documentation, the slot should be executed in the thread that "owns the method"

Correct but QThread is not a thread, it's a wrapper around it. A QThread object lives in the thread that creates it and all methods are owned by that thread, not the one spawned inside QThread. This is a common misconception, the docs are really not clear on this point. Take a look at this blog post

SGaist

@kshegunov said in Issue with QThread subclass and signal/slots:

This is what requestInterruption does internally, so I do advice making use of the already provided API. :)

It does some additional checks regarding the running status of the thread. But rest assured, I do agree that this is a the API to use.

Joel Bodenmann

@VRonin said in Issue with QThread subclass and signal/slots:

Correct but QThread is not a thread, it's a wrapper around it. A QThread object lives in the thread that creates it and all methods are owned by that thread, not the one spawned inside QThread. This is a common misconception, the docs are really not clear on this point. Take a look at this blog post

That's certainly an interesting read. However, I feel like the following information could use a bit more emphasis:

By the way, one extremely important thing to note here is that you should NEVER allocate heap objects (using new) in the constructor of the QObject class as this allocation is then performed on the main thread and not on the new QThread instance, meaning that the newly created object is then owned by the main thread and not the QThread instance.

Maybe it's just me but that seems like a very dirty pit to fall into.

Anyway, I'll rearrange things tonight and get back to you guys. Thanks for the help - much appreciated!

kshegunov

@Joel-Bodenmann said in Issue with QThread subclass and signal/slots:

Maybe it's just me but that seems like a very dirty pit to fall into.

That one is wrong. :)
You just have to give the worker object as parent to the objects you create (be it stack or heap). They'll be moved by Qt to the correct thread when the parent (i.e. the worker) is moved.

J.Hilk

@kshegunov said in Issue with QThread subclass and signal/slots:

@Joel-Bodenmann said in Issue with QThread subclass and signal/slots:

Maybe it's just me but that seems like a very dirty pit to fall into.

That one is wrong. :)
You just have to give the worker object as parent to the objects you create (be it stack or heap). They'll be moved by Qt to the correct thread when the parent (i.e. the worker) is moved.

correct me if I'm wrong, but that's only true for objects that have QObject as baseclass, right?

kshegunov

@J.Hilk said in Issue with QThread subclass and signal/slots:

correct me if I'm wrong, but that's only true for objects that have QObject as baseclass, right?

Absolutely right. Then let me respond with a counter question, what is thread affinity (i.e. in what thread does an object live in) if it's not derived from QObject? ;)

J.Hilk

@kshegunov

true,

mmh, what I could think of is maybe (member)QDataStream and QFile?

kshegunov

~~Both of which are~~ QFile is a QObject :D

J.Hilk

@kshegunov
are you sure? it has a QIODevice as a member (pointer) variable and that may or may not be derived from QObject, depending on

#ifndef QT_NO_QOBJECT
    : public QObject
#endif

But I don't see saynthing in the code pointing to QObject.
https://code.woboq.org/qt5/qtbase/src/corelib/serialization/qdatastream.h.html

kshegunov

@J.Hilk said in Issue with QThread subclass and signal/slots:

are you sure?

Yes, of course taking in mind that I rushed a bit and qobject-ified the data stream. Otherwise the QIODevice is a QObject for sure. That macro shouldn't bother you, it's there for other reasons (like QtScript). Your QIODevice c++ objects are all QObjects.

Joel Bodenmann

Okay so I reorganized everything to use moveToThread() instead of subclassing QThread and everything is working as expected.
The only thing that provided me with these two painful days was the fact that I didn't understand that slots of a QThread subclass are not executed in the new thread even though queued connections are used. I hope that this information will help others in the future.

Thank you everybody for your help! As always: Much appreciated!

JKSH

@Joel-Bodenmann said in Issue with QThread subclass and signal/slots:

The only thing that provided me with these two painful days was the fact that I didn't understand that slots of a QThread subclass are not executed in the new thread even though queued connections are used.

Hi @Joel-Bodenmann, I'm thinking of improving the QThread documentation to help developers avoid such a pitfall in the future.

Your particular scenario was mentioned in https://doc.qt.io/qt-5/qthread.html :

"It is important to remember that a QThread instance lives in the old thread that instantiated it, not in the new thread that calls run(). This means that all of QThread's queued slots and invoked methods will execute in the old thread. Thus, a developer who wishes to invoke slots in the new thread must use the worker-object approach; new slots should not be implemented directly into a subclassed QThread."

However, that section is quite buried in the documentation.

Can you think of a way to update the documentation, so that someone else in your position would be more likely to read and understand it?

kshegunov

A QThread object manages one thread of control within the program. QThreads begin executing in run(). By default, run() starts the event loop by calling exec() and runs a Qt event loop inside the thread.

To:

A QThread object is not a thread itself, instead it manages one thread of control within the program. This also implies that QThread objects, as any QObject, have a [thread affinity](url to affinity) initially determined at the time they are created and their slots are queued for execution according to their affinity. /You may want to rephrase it a bit better though/
[... continue on with the how ...] QThreads begin executing in run(). By default, run() starts the event loop by calling exec() and runs a Qt event loop inside the thread.

Joel Bodenmann

Please excuse my late reply. I wanted to think about this a bit.

I guess we all agree that the documentation is not wrong. It clearly states that slots are not executed in a separate thread when subclassing QThread. Personally, I also feel like this is not exactly "buried" in the documentation.

The main problem I see is that people that start coming across QThread are getting misleaded very easily by those famous blog posts that try to tell you how to do it, and then not to do it like that, but then again, it's apparently fine too.
I feel like the a paragraph could be added to the documentation which clearly states when subclassing QThread is needed and when not. Basically a: "Only subclass QThread if ....
Which might be followed by a "Things to keep in mind" bullet list which lists the "special properties" or "pitfalls".

At the end it really just boils down to the fact that QThread is not a thread but just a thread wrapper. From my personal experience I feel like this is very different from how other frameworks provide threading.
Of course I understand that renaming QThread is out of the question but I feel like some very generic information paragraph in form of a bullet list might help a lot.