gui: Fix shutdown order

promag commented at 1:12 am on January 29, 2019: member

This PR consists in small fixes in order to have a clean shutdown from the GUI.

fanquake added the label GUI on Jan 29, 2019

in src/qt/bitcoin.cpp:222 in 3f9a86ee85 outdated

217@@ -218,6 +218,8 @@ BitcoinApplication::~BitcoinApplication()
218 #ifdef ENABLE_WALLET
219     delete paymentServer;
220     paymentServer = nullptr;
221+    delete m_wallet_controller;
222+    m_wallet_controller = nullptr;

Sjors commented at 4:52 pm on January 29, 2019:

Can you explain this move, either in a comment or in the commit message?

promag commented at 5:41 pm on January 29, 2019:

The m_wallet_controller must be deleted after window (because it’s used there).

Sjors commented at 6:17 pm on January 29, 2019:

Thanks. I meant in a source code comment though :-) The shutdown logic keeps causing headaches, so there’s no such thing as too much documentation I think.

promag commented at 9:43 pm on January 29, 2019:

I’ll add if I need to update this branch for other reasons, otherwise I’ll improve the shutdown even further with more comments. From testing this is enough to get #15153 working without problems.

in src/qt/bitcoingui.h:99 in eb8f3ef20c outdated

94@@ -95,6 +95,9 @@ class BitcoinGUI : public QMainWindow
95     */
96     bool hasTrayIcon() const { return trayIcon; }
97 
98+    /** Disconnect core signals from GUI client */
99+    void unsubscribeFromCoreSignals();

Sjors commented at 4:54 pm on January 29, 2019:

Nit: might as well merge this with the next commit, which explains why this exposure is needed..

promag commented at 7:37 am on January 30, 2019:

I think it’s good to have move only separate?

Empact commented at 7:55 pm on January 31, 2019:

I like to combine exposing a new interface with adding uses for it - I think of it as an expression of the outside-in development principle, the use comes before the changes it motivates/justifies.

in src/qt/bitcoin.cpp:317 in 2f490ffe04 outdated

308@@ -309,14 +309,20 @@ void BitcoinApplication::requestShutdown()
309     qDebug() << __func__ << ": Requesting shutdown";
310     startThread();
311     window->hide();
312+    // Must disconnect node signals otherwise current thread can deadlock since
313+    // no event loop is running.
314+    window->unsubscribeFromCoreSignals();
315+    // Request node shutdown, which can interrupt long operations, like
316+    // rescanning a wallet.
317+    m_node.startShutdown();

Sjors commented at 4:56 pm on January 29, 2019:

Can you explain why you moved startShutdown(); up? Is it because setClientModel could be blocking and so you can to get this over with first?

promag commented at 5:40 pm on January 29, 2019:

Calling setClientModel(nullptr) hits: https://github.com/bitcoin/bitcoin/blob/7275365c9bc7e7ebd6bbf7dcb251946aac44b5de/src/qt/rpcconsole.cpp#L688-L692

and if there is a rescan in progress it blocks until the rescan completes. Calling m_node.startShutdown(); causes the rescan to interrupt.

Sjors commented at 5:01 pm on January 29, 2019: member

Concept ACK, but paging @Empact, @laanwj, @LeandroRocha84 #13217 and @MarcoFalke #13880 as QT shutdown connoisseurs.

jonasschnelli commented at 8:36 pm on January 29, 2019: contributor

utACK 870e35c30c0286e729a55fc343ab9b1945619699

in src/validationinterface.cpp:106 in 870e35c30c outdated

102@@ -103,6 +103,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) {
103 }
104 
105 void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
106+    if (!g_signals.m_internals) return;

hebasto commented at 7:24 am on January 30, 2019:

Could be

0    if (g_signals.m_internals) {
1        g_signals.m_internals->m_connMainSignals.erase(pwalletIn);
2    }

promag commented at 7:39 am on January 30, 2019:

Yes, but in other function it also early returns. Also, if the condition is removed the code doesn’t have to be re-indent.

hebasto commented at 7:46 am on January 30, 2019:

Checking a pointer variable before use is a general pattern, right? Why could it be removed?

promag commented at 10:54 pm on January 30, 2019:

Changed to your suggestion.

hebasto commented at 7:32 am on January 30, 2019: member

tACK 870e35c30c0286e729a55fc343ab9b1945619699 (Linux) modulo #15280 (review) and #15280 (review).

Empact commented at 6:13 pm on January 30, 2019: member

This could benefit from adding “why” to each commit message, which currently only describe what is done. Will be easier to evaluate now and interpret in the future with that info.

promag force-pushed on Jan 30, 2019

promag commented at 11:02 pm on January 30, 2019: member

Improved each commit message (hope it is now more clear).

A good test case to check with and without this PR is:

add a MilliSleep to the rescan loop, after the while: https://github.com/bitcoin/bitcoin/blob/cb77dc820f1bc1965a9d40759feb201d7869cfa9/src/wallet/wallet.cpp#L1659-L1660
launch with -nowallet -rescan
call loadwallet a_wallet_with_some_transactions in the console window
close the application

DrahtBot commented at 1:04 am on January 31, 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:

#15153 (gui: Add Open Wallet menu by promag)

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.

Empact commented at 7:59 pm on January 31, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15280/commits/0b53bf94d30d1a90770a86c89b97de929b0ffc27

Thanks for expanding those commit messages. Should we also guard in RegisterValidationInterface? It’s the only other unguarded access to g_signals.m_internals.

promag commented at 7:39 am on February 1, 2019: member

@Empact different PR IMO. I had a commit to guard it with cs_main but removed since it’s not GUI related.

gui: Fix WalletController deletion

The wallet controller instanced must be deleted after the window instance
since it is used there.

60e190ceb3

gui: Expose BitcoinGUI::unsubscribeFromCoreSignals

Move only change that makes unsubscribeFromCoreSignals public. It must be
called if the event loop is not running otherwise core signals handlers
can deadlock.

07b9aadcfc

gui: Fix m_node.startShutdown() order

This change forwards the shutdown request on the GUI (close the
application for instace) to the node as soon as possible. This way the
GUI doesn't have to wait for long operations to complete (rescan the
wallet for instance), instead those operations detect the shutdown
request and abort/interrupt.

fd6d499bda

Check m_internals in UnregisterValidationInterface

When a wallet is created it is registered in the validation interface (in
CWallet::CreateWalletFromFile) but it is not immediately added to the
wallets list. If a shutdown is requested before AddWallet (case more
evident when -rescan is set) then m_internals can be released (in
Shutdown -> UnregisterBackgroundSignalScheduler) before the wallet and
then ReleaseWallet would call UnregisterValidationInterface with
m_internals already released.

0dd6a8c124

promag force-pushed on Feb 3, 2019

laanwj commented at 9:49 am on February 4, 2019: member

utACK 0dd6a8c12489ea4428b398a2328dde5d1a9fe39b

pull[bot] referenced this in commit 64127b3098 on Feb 4, 2019

laanwj merged this on Feb 4, 2019

laanwj closed this on Feb 4, 2019

promag deleted the branch on Feb 4, 2019

deadalnix referenced this in commit dfaae2679d on Jun 3, 2020

ftrader referenced this in commit 0be3b725b9 on Aug 17, 2020

PastaPastaPasta referenced this in commit 87d18dc365 on Aug 2, 2021

kittywhiskers referenced this in commit b04d386d69 on Aug 3, 2021

kittywhiskers referenced this in commit cd9428de22 on Aug 8, 2021

UdjinM6 referenced this in commit f967582f53 on Aug 11, 2021

kittywhiskers referenced this in commit b84dcecbbb on Aug 12, 2021

UdjinM6 referenced this in commit 607bd4b6b5 on Aug 13, 2021

PastaPastaPasta referenced this in commit f897dbeef1 on Sep 21, 2021

kittywhiskers referenced this in commit 82f9500298 on Oct 12, 2021

MarcoFalke locked this on Dec 16, 2021

gui: Fix shutdown order #15280

Conflicts