Keep InitExecutor in main gui thread

[promag]

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).

[promag]

Best reviewed without whitespace changes.

[hebasto]

Why do you want to keep an instance of the InitExecutor in the GUI thread?

[promag]

• moveToThread is usually not called within the object;
• we don't need to hold on to the thread, we just need to call initialize and shutdown asynchronously, so we could also improve that;
• in QML it's not possible to connect/bind to objects in other threads, see Expose init executor instance gui-qml#37;

[hebasto]

Concept ACK.

[hebasto]

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

[hebasto]

Thanks! I cannot imagine the reason how it slipped in #381 🤷‍♂️

[hebasto]

Sorry, I meant dropping __func__ part only, while keeping debugging messages that seem useful (at least for me).

[promag]

No worries, done!

[hebasto]

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

[promag]

Code review ACK

[jarolrod]

ACK 03a5fe0

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.

[ryanofsky]

Code review ACK 03a5fe0. 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".

[promag]

@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 (this and this->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:

InitExecutor {
    id: executor
}

Loader {
    active: executor.ready
    sourceComponent: MySuperInterface {
    }
}

Button {
    text: "start"
    onClicked: executor.initialize()
}

This seems preferable for multiple reasons:

• no dependencies from context variables or the application
• stuff is constructed when needed, with property bindings to/from relevant places
• simplifies testing because it could mock InitExecutor

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 InitExecutor still needs to be improved.

[hebasto]

I've linked to this discussion from bitcoin-core/gui-qml#37, and suggesting to continue it there.