qt: Remove redundant WalletController::addWallet slot

hebasto commented at 7:11 pm on July 6, 2019: member

~~Fix #15453.~~ It is fixed by #16348 (comment)

The only reason of these lines on master (8c69fae94410f54bad13be0f34d54370fddbf4b3) https://github.com/bitcoin/bitcoin/blob/2679bb8919b5089f8067ccfd94f766747b8df671/src/qt/walletcontroller.cpp#L121-L128 is to Q_EMIT walletAdded(wallet_model); in a thread-safe manner;

This PR makes this in a line of code: https://github.com/bitcoin/bitcoin/blob/1b83875006749d79916af0197bed65aecdc7ff17/src/qt/walletcontroller.cpp#L121

EDITED: To establish the ownership of a new WalletModel object is not necessary on the master (https://github.com/bitcoin/bitcoin/pull/16349#discussion_r301679192 by promag). But:

it’s good habit to set ownership

And I agree. It is a safe practice.

fanquake added the label GUI on Jul 6, 2019

fanquake added the label Wallet on Jul 6, 2019

DrahtBot commented at 9:19 pm on July 6, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto renamed this:
~~Fix "opening a wallet" issue #15453~~
qt: Fix "opening a wallet" issue #15453
on Jul 7, 2019

promag commented at 10:35 pm on July 7, 2019: member

Can you explain why it fixes?

fanquake commented at 1:01 am on July 8, 2019: member

Concept ACK. I’ve tested that this PR fixes #15453 (running with -nowallet on Debian with Qt 5.7.1). However I haven’t yet checked for any regressions.

I agree with promag that more of an explanation (also added to the PR body) would be good. Can you also try improve the commit title.

hebasto commented at 6:08 am on July 8, 2019: member

@promag

Can you explain why it fixes? @fanquake I agree with promag that more of an explanation (also added to the PR body) would be good.

Trying to investigate the #15453 bug I’ve downloaded Qt 5.7.1, but failed to build the Bitcoin Core. I even failed to start Qt Creator (there are some similar messages on Qt bug tracker). So, unfortunately, I have no clear explanation of such Qt 5.7.1 behavior for now.

Regarding the code patch: the private slot addWallet() seems redundant in the master branch.

hebasto commented at 6:09 am on July 8, 2019: member

@fanquake

Can you also try improve the commit title.

With my pleasure. What title could you suggest?

fanquake commented at 12:05 pm on July 8, 2019: member

What title could you suggest?

Maybe: qt: refactor WalletController to fix loading wallets when running with -nowallet.

Then you could extend the body text to be:

This establishes a parent-child relation between WalletController and WalletModel objects immediately. This fixes wallet loading on Debian when building against Qt 5.7.x and running with -nowallet.

MarcoFalke commented at 12:37 pm on July 8, 2019: member

So this is only an issue with qt5.7.x?

If so, it could be tested by reverting depends to 2fca6568f27c9a1714b7a415cde947b79c10ed45 locally.

hebasto commented at 4:49 pm on July 8, 2019: member

@promag regarding the IRC discussion:

you are definitely right: a WalletController instance thread affinity and the current running thread could (and mostly are) different;
the only reason of these lines https://github.com/bitcoin/bitcoin/blob/2679bb8919b5089f8067ccfd94f766747b8df671/src/qt/walletcontroller.cpp#L121-L128 is to Q_EMIT walletAdded(wallet_model); in a thread-safe manner;
in this PR the next line is also thread-safe by default: https://github.com/bitcoin/bitcoin/blob/1b83875006749d79916af0197bed65aecdc7ff17/src/qt/walletcontroller.cpp#L121

hebasto renamed this:
~~qt: Fix "opening a wallet" issue #15453~~
qt: Remove redundant WalletController::addWallet signal
on Jul 8, 2019

hebasto force-pushed on Jul 8, 2019

hebasto commented at 5:18 pm on July 8, 2019: member

As #15453 is fixed in #16348 (comment) I have updated the OP and the commit message.

Please, label this PR as refactoring.

fanquake added the label Refactoring on Jul 8, 2019

promag commented at 0:37 am on July 9, 2019: member

Concept ACK, will test.

promag commented at 8:18 am on July 9, 2019: member

This is useful for reviewers: https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qobject.cpp.html#_ZN7QObject12moveToThreadEP7QThread

hebasto force-pushed on Jul 9, 2019

hebasto commented at 12:55 pm on July 9, 2019: member

Rebased on top of #16348.

hebasto commented at 12:58 pm on July 9, 2019: member

@ryanofsky Would you mind reviewing this PR?

in src/qt/walletcontroller.cpp:125 in 225ff6b28a outdated

120@@ -119,25 +121,11 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
121     connect(wallet_model, &WalletModel::coinsSent, this, &WalletController::coinsSent);
122 
123     // Notify walletAdded signal on the GUI thread.
124-    if (QThread::currentThread() == thread()) {
125-        addWallet(wallet_model);
126-    } else {
127-        // Handler callback runs in a different thread so fix wallet model thread affinity.

promag commented at 1:05 pm on July 9, 2019:

Could keep this comment.

hebasto commented at 1:27 pm on July 9, 2019:

Done.

in src/qt/walletcontroller.cpp:125 in 225ff6b28a outdated

127-        // Handler callback runs in a different thread so fix wallet model thread affinity.
128-        wallet_model->moveToThread(thread());
129-        bool invoked = QMetaObject::invokeMethod(this, "addWallet", Qt::QueuedConnection, Q_ARG(WalletModel*, wallet_model));
130-        assert(invoked);
131-    }
132+    Q_EMIT walletAdded(wallet_model);

promag commented at 1:08 pm on July 9, 2019:

Just to be clear, this would work even without qRegisterMetaType<WalletModel*>() added in #16348?

hebasto commented at 1:19 pm on July 9, 2019:

Yes.

hebasto force-pushed on Jul 9, 2019

promag commented at 1:29 pm on July 9, 2019: member

s/signal/slot in PR title and commit message?

Remove redundant WalletController::addWallet slot 6285a318d7

hebasto force-pushed on Jul 9, 2019

hebasto renamed this:
~~qt: Remove redundant WalletController::addWallet signal~~
qt: Remove redundant WalletController::addWallet slot
on Jul 9, 2019

hebasto commented at 1:33 pm on July 9, 2019: member

@promag

s/signal/slot in PR title and commit message?

Sure ;) Thank you. Done.

promag commented at 1:57 pm on July 9, 2019: member

ACK 6285a318d77dbfdf50f893963ebfb2973746f757.

in src/qt/walletcontroller.cpp:102 in 6285a318d7

 98@@ -99,6 +99,9 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
 99 
100     // Instantiate model and register it.
101     WalletModel* wallet_model = new WalletModel(std::move(wallet), m_node, m_platform_style, m_options_model, nullptr);
102+    // Handler callback runs in a different thread so fix wallet model thread affinity.

ryanofsky commented at 2:55 pm on July 9, 2019:

This comment is very vague. In particular it’s not clear what “handler callback runs in a different thread” is referring to. If I’m understanding this correctly, I think a comment that specifically says which threads the object is being moved to and from would be more helpful:

0// Move WalletModel object to the thread that created the WalletController
1// object (GUI main thread), instead of the current thread, which could be
2// an outside wallet thread or RPC thread sending a LoadWallet notification.
3//
4// This ensures queued signals sent to the WalletModel object will be
5// handled on the GUI event loop, see
6// https://doc.qt.io/qt-5/threads-qobject.html#signals-and-slots-across-threads

hebasto commented at 5:40 pm on July 13, 2019:

This comment is very vague.

Agree, but I’d leave it to preserve ACKs.

in src/qt/walletcontroller.cpp:104 in 6285a318d7

 98@@ -99,6 +99,9 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
 99 
100     // Instantiate model and register it.
101     WalletModel* wallet_model = new WalletModel(std::move(wallet), m_node, m_platform_style, m_options_model, nullptr);
102+    // Handler callback runs in a different thread so fix wallet model thread affinity.
103+    wallet_model->moveToThread(thread());
104+    wallet_model->setParent(this);

ryanofsky commented at 3:16 pm on July 9, 2019:

Curious: Is this setParent call actually necessary for anything? Maybe consider dropping it or adding a comment that says why it’s useful.

If you will keep this line, it’d also be helpful to have a comment explaining why we are passing parent=nullptr to the constructor above, and then changing the parent later. I am guessing from https://doc.qt.io/qt-5/qobject.html#thread-affinity, that it is because “All QObjects must live in the same thread as their parent” and so this is necessary to avoid a “QObject: Cannot create children for a parent that is in a different thread” error.

promag commented at 4:25 pm on July 9, 2019:

Exactly, can’t set parent on constructor because parent thread can be different.

setParent is not necessary at the moment because (afaict) the wallet is always unloaded here: https://github.com/bitcoin/bitcoin/blob/8046a3e0befeea641b6309bc0c742b7481e681d9/src/qt/walletcontroller.cpp#L104

However I think it’s good habit to set ownership, so at least the following would release all models: https://github.com/bitcoin/bitcoin/blob/8046a3e0befeea641b6309bc0c742b7481e681d9/src/qt/bitcoin.cpp#L216

hebasto commented at 5:33 pm on July 13, 2019:

PR description has been updated.

in src/qt/walletcontroller.cpp:124 in 6285a318d7

121@@ -119,25 +122,11 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
122     connect(wallet_model, &WalletModel::coinsSent, this, &WalletController::coinsSent);
123 
124     // Notify walletAdded signal on the GUI thread.

ryanofsky commented at 3:32 pm on July 9, 2019:

It seems like this comment is obsolete and should be dropped.

hebasto commented at 5:36 pm on July 13, 2019:

Agree, but I’d leave it to preserve ACKs.

in src/qt/walletcontroller.cpp:127 in 6285a318d7

121@@ -119,25 +122,11 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
122     connect(wallet_model, &WalletModel::coinsSent, this, &WalletController::coinsSent);
123 
124     // Notify walletAdded signal on the GUI thread.
125-    if (QThread::currentThread() == thread()) {
126-        addWallet(wallet_model);
127-    } else {
128-        // Handler callback runs in a different thread so fix wallet model thread affinity.
129-        wallet_model->moveToThread(thread());
130-        bool invoked = QMetaObject::invokeMethod(this, "addWallet", Qt::QueuedConnection, Q_ARG(WalletModel*, wallet_model));

ryanofsky commented at 3:34 pm on July 9, 2019:

It seems good to get rid of this, but I can’t figure out why it was ever necessary to call addWallet here with invokeMethod instead of directly.

ryanofsky approved

ryanofsky commented at 4:09 pm on July 9, 2019: member

Code review ACK d665a5bd8c2df38151dd9c2642ef0be14eaf4825. This seems good as a pure code simplification since the previous code was wild, but the PR description isn’t understandable to me, and I can’t figure out what the thread affinities and parent child associations are used for here. I’d recommend rewriting the PR description to reflect current state of things, and adding better comments.

Specifically, I don’t understand why the WalletController::setParent call is necessary. Where are parents and children used here? If the setParent call isn’t necessary, then the moveToThread call doesn’t seem necessary either. I don’t see any signals being sent to WalletController objects, only signals emitted from WalletController objects, so I don’t know how WalletController thread affinity would be relevant to anything.

In general, it seems not ideal that wallet and RPC threads would call GUI methods and create QObjects. It would be better if wallet notifications just emitted signals that got handled asynchronously in the GUI. That way, wallet code wouldn’t be blocked and GUI code wouldn’t have to be written to deal with external threads.

promag commented at 4:36 pm on July 9, 2019: member

It would be better if wallet notifications just emitted signals that got handled asynchronously in the GUI. @ryanofsky IIRC I had some problems with this because of std::unique_ptr<interfaces::Wallet>, it was easier to instantiate the WalletModel and then send to the GUI thread by invoking addWallet.

From #16349 (review)

It seems good to get rid of this, but I can’t figure out why it was ever necessary to call addWallet here with invokeMethod instead of directly.

At the time the idea was to allow (and prevent issues) connect(..., &WalletController::walletAdded, ..., Qt::DirectConnection).

But agree, this code can be improved.

ryanofsky approved

ryanofsky commented at 5:26 pm on July 15, 2019: member

utACK 6285a318d77dbfdf50f893963ebfb2973746f757. Only change since last review is rebasing and restoring a deleted comment. I do think the comments I suggested last review would be better than this one, but this is at least better than before.

jonasschnelli commented at 12:15 pm on August 12, 2019: contributor

utACK 6285a318d77dbfdf50f893963ebfb2973746f757

jonasschnelli merged this on Aug 12, 2019

jonasschnelli closed this on Aug 12, 2019

pull[bot] referenced this in commit 9059a6f248 on Aug 12, 2019

hebasto deleted the branch on Aug 12, 2019

sidhujag referenced this in commit 4bea2828a2 on Aug 13, 2019

deadalnix referenced this in commit d7437bcdf7 on May 21, 2020

ShengguangXiao referenced this in commit 66d822db4b on Aug 28, 2020

kittywhiskers referenced this in commit d39e42ede2 on Nov 6, 2021

Munkybooty referenced this in commit 158061f4f2 on Nov 25, 2021

Munkybooty referenced this in commit eedd48926c on Nov 30, 2021

DrahtBot locked this on Dec 16, 2021

qt: Remove redundant WalletController::addWallet slot #16349

Conflicts