Issue with QThread subclass and signal/slots
-
@kshegunov Nice catches ! I did miss some interesting cases...
If you insist on keeping
_stopRequested, you should make it anstd::atomicotherwise, you might get surprising optimisation depending on how you access it.@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
requestInterruptiondoes internally, so I do advice making use of the already provided API. :)Read and abide by the Qt Code of Conduct
-
@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 ifQt::DirectConnectionis used. The way I understand the documentation, the slot should be executed in the thread that "owns the method" ifQt::QueuedConnectionis 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
FindControlleris 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 outQThread::requestInterruption()andQThread::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.
@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
-
@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 ifQt::DirectConnectionis used. The way I understand the documentation, the slot should be executed in the thread that "owns the method" ifQt::QueuedConnectionis 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
FindControlleris 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 outQThread::requestInterruption()andQThread::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.
@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
QThreadis not a thread, it's a wrapper around it. AQThreadobject lives in the thread that creates it and all methods are owned by that thread, not the one spawned insideQThread. This is a common misconception, the docs are really not clear on this point. Take a look at this blog post -
@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
requestInterruptiondoes internally, so I do advice making use of the already provided API. :)@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 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
QThreadis not a thread, it's a wrapper around it. AQThreadobject lives in the thread that creates it and all methods are owned by that thread, not the one spawned insideQThread. This is a common misconception, the docs are really not clear on this point. Take a look at this blog post@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!
Industrial process automation software: https://simulton.com
Embedded Graphics & GUI library: https://ugfx.io -
@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!
@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.Read and abide by the Qt Code of Conduct
-
@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.@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?
Be aware of the Qt Code of Conduct, when posting : https://forum.qt.io/topic/113070/qt-code-of-conduct
Q: What's that?
A: It's blue light.
Q: What does it do?
A: It turns blue. -
@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?
@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? ;)Read and abide by the Qt Code of Conduct
-
@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? ;)
Be aware of the Qt Code of Conduct, when posting : https://forum.qt.io/topic/113070/qt-code-of-conduct
Q: What's that?
A: It's blue light.
Q: What does it do?
A: It turns blue. -
-
@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 #endifBut I don't see saynthing in the code pointing to QObject.
https://code.woboq.org/qt5/qtbase/src/corelib/serialization/qdatastream.h.html
Be aware of the Qt Code of Conduct, when posting : https://forum.qt.io/topic/113070/qt-code-of-conduct
Q: What's that?
A: It's blue light.
Q: What does it do?
A: It turns blue. -
@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 #endifBut I don't see saynthing in the code pointing to QObject.
https://code.woboq.org/qt5/qtbase/src/corelib/serialization/qdatastream.h.html@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
QIODeviceis aQObjectfor sure. That macro shouldn't bother you, it's there for other reasons (like QtScript). YourQIODevicec++ objects are allQObjects. -
Okay so I reorganized everything to use
moveToThread()instead of subclassingQThreadand 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 aQThreadsubclass 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!
Industrial process automation software: https://simulton.com
Embedded Graphics & GUI library: https://ugfx.io -
Okay so I reorganized everything to use
moveToThread()instead of subclassingQThreadand 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 aQThreadsubclass 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!
@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
QThreadsubclass 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?
Qt Doc Search for browsers: forum.qt.io/topic/35616/web-browser-extension-for-improved-doc-searches
-
@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
QThreadsubclass 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?
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. -
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
QThreadare 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 subclassingQThreadis 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
QThreadis 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 renamingQThreadis out of the question but I feel like some very generic information paragraph in form of a bullet list might help a lot.
