The InitExecutor constructor moves the instance to a dedicated thread. This PR changes that by using GUIUtil::ObjectInvoke to run the relevant code in that thread.
A possible follow-up is to ditch the dedicated thread and use QThreadPool or even QtConcurrent::run (if we want to enable that).
InitExecutor in the GUI thread?
moveToThread is usually not called within the object;
51- handleRunawayException(nullptr);
52- }
53+ GUIUtil::ObjectInvoke(&m_context, [this] {
54+ try {
55+ util::ThreadRename("qt-init");
56+ qDebug() << __func__ << ": Running initialization in thread";
__func__ identifier is operator(). Maybe just drop it?
8@@ -9,7 +9,6 @@
9 #include <interfaces/init.h>
10 #include <interfaces/node.h>
11 #include <qt/bitcoin.h>
12-#include <qt/initexecutor.h>
2@@ -3,6 +3,7 @@
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4
5 #include <qt/initexecutor.h>
6+#include <qt/guiutil.h>
Could you move this header in the main group?
0#include <interfaces/node.h>
1#include <qt/guiutil.h>
2#include <util/system.h>
3#include <util/threadnames.h>
Approach ACK 45b1c7745eecaa544e2da12c628b8e3b95e420fe, tested on Linux Mint 20.2 (Qt 5.12.8):
0$ src/qt/bitcoin-qt -signet -printtoconsole -debug=qt
1...
22021-09-27T11:15:47Z [main] GUI: requestInitialize : Requesting initialize
32021-09-27T11:15:47Z [qt-init] GUI: operator() : Running initialization in thread
4...
52021-09-27T11:15:50Z [main] GUI: requestShutdown : Requesting shutdown
62021-09-27T11:15:50Z [qt-init] GUI: operator() : Running Shutdown in thread
7...
82021-09-27T11:15:51Z [shutoff] Shutdown: done
92021-09-27T11:15:51Z [shutoff] GUI: operator() : Shutdown finished
102021-09-27T11:15:51Z [main] GUI: ~InitExecutor : Stopping thread
112021-09-27T11:15:51Z [main] GUI: ~InitExecutor : Stopped thread
__func__ part only, while keeping debugging messages that seem useful (at least for me).
ACK 03a5fe06bd85111ef844a30dcfdf3b317ff74517, tested on Linux Mint 20.2 (Qt 5.12.8).
The last lines in the log:
0...
12021-09-27T14:14:55Z [shutoff] Shutdown: done
22021-09-27T14:14:55Z [shutoff] GUI: Shutdown finished
32021-09-27T14:14:55Z [main] GUI: ~InitExecutor : Stopping thread
42021-09-27T14:14:55Z [main] GUI: ~InitExecutor : Stopped thread
ACK 03a5fe06bd85111ef844a30dcfdf3b317ff74517
I’ve tested this and have reviewed the code. Keeping a QObject that will serve as a marker into a thread is a good design decision going forward.
Code review ACK 03a5fe06bd85111ef844a30dcfdf3b317ff74517. Having InitExecutor associated with the GUI thread seems to be necessary in order to be able to write Connections { target: initExecutor ... } in QML.
But this seems like an overly complicated design because now instead of InitExecutor just being an QObject/QThread couple it is QObject/QThread/nested QObject throuple with two QObjects (this and this->m_context) each QObject connected to different thread.
It seems like a mistake here is letting QML code be aware of the InitExecutor object at all. Probably QML could should just be sending / receiving init events to and from a node or application object. QML code should not care about how init code executes or whether or not there is an executor. QML code should just be saying “hey bitcoin application, please start initializing and send the initialization result to this place”, not “hey bitcoin application, give me your init executor, and init exector, please start initializing and send the result to this place”.
@ryanofsky thanks for taking a look.
But this seems like an overly complicated design because now instead of InitExecutor just being an QObject/QThread couple it is QObject/QThread/nested QObject throuple with two QObjects (
thisandthis->m_context) each QObject connected to different thread.
Implementation can change - how it runs async code. Personally, I don’t think holding the thread makes sense, it’s not used while the app runs.
It seems like a mistake here is letting QML code be aware of the InitExecutor object at all. Probably QML could should just be sending / receiving init events to and from a node or application object. QML code should not care about how init code executes or whether or not there is an executor. QML code should just be saying “hey bitcoin application, please start initializing and send the initialization result to this place”, not “hey bitcoin application, give me your init executor, and init exector, please start initializing and send the result to this place”.
We need one or some objects to bind to but I don’t think the application is the best candidate. We could have some fat object with all properties and signals and just expose it. Later we could split that fat object into multiple objects and expose them.
In fact, QML allows different approaches. What you and @hebasto suggest seems to be https://doc.qt.io/archives/qt-5.9/qtqml-cppintegration-contextproperties.html. On the other hand, I’d prefer to follow https://doc.qt.io/archives/qt-5.9/qtqml-cppintegration-definetypes.html.
Following the example you mention, I was thinking something like:
0InitExecutor {
1 id: executor
2}
3
4Loader {
5 active: executor.ready
6 sourceComponent: MySuperInterface {
7 }
8}
9
10Button {
11 text: "start"
12 onClicked: executor.initialize()
13}
This seems preferable for multiple reasons:
InitExecutorI don’t think this violates any principle. In this case, we still have the init/shutdown logic implemented in C++ and the UI in QML.
But I’m fine with any approach, I’m just expressing my preference. Regardless of how we structure the work in QML, I think InitExecutor still needs to be improved.
Labels
Refactoring