qt: Remove redundant WalletController::addWallet slot #16349

hebasto · 2019-07-06T19:11:16Z

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

The only reason of these lines on master (8c69fae)

Lines 121 to 128 in 2679bb8

    
           // Notify walletAdded signal on the GUI thread. 
        
           if (QThread::currentThread() == thread()) { 
        
               addWallet(wallet_model); 
        
           } else { 
        
               // Handler callback runs in a different thread so fix wallet model thread affinity. 
        
               wallet_model->moveToThread(thread()); 
        
               QMetaObject::invokeMethod(this, "addWallet", Qt::QueuedConnection, Q_ARG(WalletModel*, wallet_model)); 
        
           }

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 (#16349 (comment) by promag).
But:

it's good habit to set ownership

And I agree. It is a safe practice.

DrahtBot · 2019-07-06T21:19:06Z

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.

promag · 2019-07-07T22:35:04Z

Can you explain why it fixes?

fanquake · 2019-07-08T01:01:40Z

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 · 2019-07-08T06:08:37Z

@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 · 2019-07-08T06:09:48Z

@fanquake

Can you also try improve the commit title.

With my pleasure. What title could you suggest?

fanquake · 2019-07-08T12:05:22Z

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.

maflcko · 2019-07-08T12:37:29Z

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

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

hebasto · 2019-07-08T16:49:27Z

@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

bitcoin/src/qt/walletcontroller.cpp

Lines 121 to 128 in 2679bb8

    
           // Notify walletAdded signal on the GUI thread. 
        
           if (QThread::currentThread() == thread()) { 
        
               addWallet(wallet_model); 
        
           } else { 
        
               // Handler callback runs in a different thread so fix wallet model thread affinity. 
        
               wallet_model->moveToThread(thread()); 
        
               QMetaObject::invokeMethod(this, "addWallet", Qt::QueuedConnection, Q_ARG(WalletModel*, wallet_model)); 
        
           }

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 · 2019-07-08T17:18:52Z

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

Please, label this PR as refactoring.

promag

Concept ACK, will test.

promag · 2019-07-09T08:18:37Z

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

hebasto · 2019-07-09T12:55:12Z

Rebased on top of #16348.

hebasto · 2019-07-09T12:58:21Z

@ryanofsky Would you mind reviewing this PR?

promag · 2019-07-09T13:05:58Z

src/qt/walletcontroller.cpp

Could keep this comment.

promag · 2019-07-09T13:08:45Z

src/qt/walletcontroller.cpp

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

promag · 2019-07-09T13:29:33Z

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

hebasto · 2019-07-09T13:33:05Z

@promag

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

Sure ;) Thank you.
Done.

promag · 2019-07-09T13:57:13Z

ACK 6285a31.

ryanofsky

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.

ryanofsky · 2019-07-09T14:55:21Z

src/qt/walletcontroller.cpp


    // Instantiate model and register it.
    WalletModel* wallet_model = new WalletModel(std::move(wallet), m_node, m_platform_style, m_options_model, nullptr);
+    // Handler callback runs in a different thread so fix wallet model thread affinity.


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:

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

This comment is very vague.

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

ryanofsky · 2019-07-09T15:16:47Z

src/qt/walletcontroller.cpp

    WalletModel* wallet_model = new WalletModel(std::move(wallet), m_node, m_platform_style, m_options_model, nullptr);
+    // Handler callback runs in a different thread so fix wallet model thread affinity.
+    wallet_model->moveToThread(thread());
+    wallet_model->setParent(this);


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.

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:

bitcoin/src/qt/walletcontroller.cpp

Line 104 in 8046a3e

connect(wallet_model, &WalletModel::unload, [this, wallet_model] {

However I think it's good habit to set ownership, so at least the following would release all models:

bitcoin/src/qt/bitcoin.cpp

Line 216 in 8046a3e

delete m_wallet_controller;

PR description has been updated.

ryanofsky · 2019-07-09T15:32:26Z

src/qt/walletcontroller.cpp

    // Re-emit coinsSent signal from wallet model.
    connect(wallet_model, &WalletModel::coinsSent, this, &WalletController::coinsSent);

    // Notify walletAdded signal on the GUI thread.


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

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

ryanofsky · 2019-07-09T15:34:41Z

src/qt/walletcontroller.cpp

-    } else {
-        // Handler callback runs in a different thread so fix wallet model thread affinity.
-        wallet_model->moveToThread(thread());
-        bool invoked = QMetaObject::invokeMethod(this, "addWallet", Qt::QueuedConnection, Q_ARG(WalletModel*, wallet_model));


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.

promag · 2019-07-09T16:36:17Z

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 (comment)

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

utACK 6285a31. 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 · 2019-08-12T12:15:26Z

utACK 6285a31

… slot 6285a31 Remove redundant WalletController::addWallet slot (Hennadii Stepanov) Pull request description: ~~Fix bitcoin#15453.~~ It is fixed by bitcoin#16348 (comment) The _only_ reason of these lines on master (8c69fae) 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 (bitcoin#16349 (comment) by **promag**). But: > it's good habit to set ownership And I agree. It is a safe practice. ACKs for top commit: promag: ACK 6285a31. jonasschnelli: utACK 6285a31 ryanofsky: utACK 6285a31. 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. Tree-SHA512: 90370cb1fe853b84dd16c3781ba4f97f3f4deca56bba0203e457f37b3220fd13228cf8495fd882ff18b7c782c27544cc2e7a88aaec5b69b9ef6d8626bdaaf332

Summary: bitcoin/bitcoin@6285a31 --- Depends on D6183 Backport of Core [[bitcoin/bitcoin#16349 | PR16349]] Test Plan: ninja check ./srt/qt/bitcoin-qt -regtest check the UI works as expected, close the default wallet, open the default wallet Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6184

1a85aa3 Remove redundant WalletController::addWallet slot (Hennadii Stepanov) Pull request description: ~~Fix #15453.~~ It is fixed by bitcoin/bitcoin#16348 (comment) The _only_ reason of these lines on master (03465aa) https://github.com/bitcoin/bitcoin/blob/8605f6a4c7cc8cdbe7eb72600b62fa18abba372e/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 (bitcoin/bitcoin#16349 (comment) by **promag**). But: > it's good habit to set ownership And I agree. It is a safe practice. ACKs for top commit: promag: ACK 1a85aa3. jonasschnelli: utACK 1a85aa3 ryanofsky: utACK 1a85aa3. 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. Tree-SHA512: 90370cb1fe853b84dd16c3781ba4f97f3f4deca56bba0203e457f37b3220fd13228cf8495fd882ff18b7c782c27544cc2e7a88aaec5b69b9ef6d8626bdaaf332

… slot 6285a31 Remove redundant WalletController::addWallet slot (Hennadii Stepanov) Pull request description: ~~Fix bitcoin#15453.~~ It is fixed by bitcoin#16348 (comment) The _only_ reason of these lines on master (8c69fae) 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 (bitcoin#16349 (comment) by **promag**). But: > it's good habit to set ownership And I agree. It is a safe practice. ACKs for top commit: promag: ACK 6285a31. jonasschnelli: utACK 6285a31 ryanofsky: utACK 6285a31. 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. Tree-SHA512: 90370cb1fe853b84dd16c3781ba4f97f3f4deca56bba0203e457f37b3220fd13228cf8495fd882ff18b7c782c27544cc2e7a88aaec5b69b9ef6d8626bdaaf332

fanquake added GUI Wallet labels Jul 6, 2019

hebasto changed the title ~~Fix "opening a wallet" issue #15453~~ qt: Fix "opening a wallet" issue #15453 Jul 7, 2019

hebasto mentioned this pull request Jul 8, 2019

qt: Assert QMetaObject::invokeMethod result #16348

Merged

hebasto changed the title ~~qt: Fix "opening a wallet" issue #15453~~ qt: Remove redundant WalletController::addWallet signal Jul 8, 2019

hebasto force-pushed the 20190706-fix15453 branch from 1b83875 to d665a5b Compare July 8, 2019 17:16

fanquake added the Refactoring label Jul 8, 2019

promag reviewed Jul 9, 2019

View reviewed changes

hebasto force-pushed the 20190706-fix15453 branch from d665a5b to 225ff6b Compare July 9, 2019 12:52

promag reviewed Jul 9, 2019

View reviewed changes

hebasto force-pushed the 20190706-fix15453 branch from 225ff6b to 41c900a Compare July 9, 2019 13:26

Remove redundant WalletController::addWallet slot

6285a31

hebasto force-pushed the 20190706-fix15453 branch from 41c900a to 6285a31 Compare July 9, 2019 13:32

hebasto changed the title ~~qt: Remove redundant WalletController::addWallet signal~~ qt: Remove redundant WalletController::addWallet slot Jul 9, 2019

ryanofsky approved these changes Jul 9, 2019

View reviewed changes

ryanofsky approved these changes Jul 15, 2019

View reviewed changes

jonasschnelli merged commit 6285a31 into bitcoin:master Aug 12, 2019

hebasto deleted the 20190706-fix15453 branch August 12, 2019 15:54

hebasto mentioned this pull request May 11, 2020

qt: Call setParent() in the parent's context #18948

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

	// Notify walletAdded signal on the GUI thread.
	if (QThread::currentThread() == thread()) {
	addWallet(wallet_model);
	} else {
	// Handler callback runs in a different thread so fix wallet model thread affinity.
	wallet_model->moveToThread(thread());
	QMetaObject::invokeMethod(this, "addWallet", Qt::QueuedConnection, Q_ARG(WalletModel*, wallet_model));
	}

qt: Remove redundant WalletController::addWallet slot #16349

qt: Remove redundant WalletController::addWallet slot #16349

Uh oh!

Conversation

hebasto commented Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Jul 7, 2019

Uh oh!

fanquake commented Jul 8, 2019

Uh oh!

hebasto commented Jul 8, 2019

Uh oh!

hebasto commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jul 8, 2019

Uh oh!

maflcko commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 8, 2019

Uh oh!

hebasto commented Jul 8, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Jul 9, 2019

Uh oh!

hebasto commented Jul 9, 2019

Uh oh!

hebasto commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jul 9, 2019

Uh oh!

hebasto commented Jul 9, 2019

Uh oh!

promag commented Jul 9, 2019

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jul 9, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 12, 2019

Uh oh!

Reviewers

hebasto commented Jul 6, 2019 •

edited

Loading

DrahtBot commented Jul 6, 2019 •

edited

Loading

hebasto commented Jul 8, 2019 •

edited

Loading

maflcko commented Jul 8, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading