Keep InitExecutor in main gui thread#434
Conversation
|
Best reviewed without whitespace changes. |
|
Why do you want to keep an instance of the |
|
|
Concept ACK. |
hebasto
left a comment
There was a problem hiding this comment.
Approach ACK 45b1c77, tested on Linux Mint 20.2 (Qt 5.12.8):
$ src/qt/bitcoin-qt -signet -printtoconsole -debug=qt
...
2021-09-27T11:15:47Z [main] GUI: requestInitialize : Requesting initialize
2021-09-27T11:15:47Z [qt-init] GUI: operator() : Running initialization in thread
...
2021-09-27T11:15:50Z [main] GUI: requestShutdown : Requesting shutdown
2021-09-27T11:15:50Z [qt-init] GUI: operator() : Running Shutdown in thread
...
2021-09-27T11:15:51Z [shutoff] Shutdown: done
2021-09-27T11:15:51Z [shutoff] GUI: operator() : Shutdown finished
2021-09-27T11:15:51Z [main] GUI: ~InitExecutor : Stopping thread
2021-09-27T11:15:51Z [main] GUI: ~InitExecutor : Stopped thread
| #include <interfaces/init.h> | ||
| #include <interfaces/node.h> | ||
| #include <qt/bitcoin.h> | ||
| #include <qt/initexecutor.h> |
There was a problem hiding this comment.
Thanks! I cannot imagine the reason how it slipped in #381 🤷♂️
45b1c77 to
14acd74
Compare
|
Sorry, I meant dropping |
14acd74 to
03a5fe0
Compare
|
No worries, done! |
hebasto
left a comment
There was a problem hiding this comment.
ACK 03a5fe0, tested on Linux Mint 20.2 (Qt 5.12.8).
The last lines in the log:
...
2021-09-27T14:14:55Z [shutoff] Shutdown: done
2021-09-27T14:14:55Z [shutoff] GUI: Shutdown finished
2021-09-27T14:14:55Z [main] GUI: ~InitExecutor : Stopping thread
2021-09-27T14:14:55Z [main] GUI: ~InitExecutor : Stopped thread
|
Code review ACK |
|
Code review ACK 03a5fe0. Having InitExecutor associated with the GUI thread seems to be necessary in order to be able to write 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 ( 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.
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.
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: InitExecutor {
id: executor
}
Loader {
active: executor.ready
sourceComponent: MySuperInterface {
}
}
Button {
text: "start"
onClicked: executor.initialize()
}This seems preferable for multiple reasons:
I 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 |
|
I've linked to this discussion from bitcoin-core/gui-qml#37, and suggesting to continue it there. |
The
InitExecutorconstructor moves the instance to a dedicated thread. This PR changes that by usingGUIUtil::ObjectInvoketo run the relevant code in that thread.A possible follow-up is to ditch the dedicated thread and use
QThreadPoolor evenQtConcurrent::run(if we want to enable that).