Issue with QThread subclass and signal/slots

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.