gui: Fix shutdown order #15280

promag · 2019-01-29T01:12:40Z

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

Sjors

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

Sjors · 2019-01-29T16:52:26Z

src/qt/bitcoin.cpp

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

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

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.

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.

Sjors · 2019-01-29T16:54:32Z

src/qt/bitcoingui.h

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

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

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.

Sjors · 2019-01-29T16:56:34Z

src/qt/bitcoin.cpp

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?

Calling setClientModel(nullptr) hits:

bitcoin/src/qt/rpcconsole.cpp

Lines 688 to 692 in 7275365

if (!model) {

// Client model is being set to 0, this means shutdown() is about to be called.

thread.quit();

thread.wait();

}

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

jonasschnelli · 2019-01-29T20:36:40Z

utACK 870e35c30c0286e729a55fc343ab9b1945619699

src/validationinterface.cpp

hebasto · 2019-01-30T07:32:27Z

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

Empact · 2019-01-30T18:13:01Z

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 · 2019-01-30T23:02:55Z

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:

bitcoin/src/wallet/wallet.cpp

Lines 1659 to 1660 in cb77dc8

    
           while (block_height && !fAbortRescan && !ShutdownRequested()) { 
        
               if (*block_height % 100 == 0 && progress_end - progress_begin > 0.0) {

launch with -nowallet -rescan
call loadwallet a_wallet_with_some_transactions in the console window
close the application

DrahtBot · 2019-01-31T01:04:16Z

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 · 2019-01-31T19:59:50Z

utACK 0b53bf9

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 · 2019-02-01T07:39:50Z

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

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

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

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.

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.

laanwj · 2019-02-04T09:49:25Z

utACK 0dd6a8c

0dd6a8c Check m_internals in UnregisterValidationInterface (João Barbosa) fd6d499 gui: Fix m_node.startShutdown() order (João Barbosa) 07b9aad gui: Expose BitcoinGUI::unsubscribeFromCoreSignals (João Barbosa) 60e190c gui: Fix WalletController deletion (João Barbosa) Pull request description: This PR consists in small fixes in order to have a clean shutdown from the GUI. Tree-SHA512: a9c641f202bc810698c4a39d5c5a1f54e54bdab098c412d65418879e00764a9db9f38383813914d591e24e097e49f177942b2ae6c57bba05dcc095e8a1d0b8f4

Summary: * gui: Fix WalletController deletion The wallet controller instanced must be deleted after the window instance since it is used there. * 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. * 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. * 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. This is a backport of Core [[bitcoin/bitcoin#15280 | PR15280]] Test Plan: ninja all check-all Run bitcoin-qt on win64 and verify it shuts down properly. Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6344

…ionInterface 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.

merge bitcoin#15118, bitcoin#14172, bitcoin#15623, bitcoin#14121, partial bitcoin#13743, partial bitcoin#15280: block filters

0dd6a8c Check m_internals in UnregisterValidationInterface (João Barbosa) fd6d499 gui: Fix m_node.startShutdown() order (João Barbosa) 07b9aad gui: Expose BitcoinGUI::unsubscribeFromCoreSignals (João Barbosa) 60e190c gui: Fix WalletController deletion (João Barbosa) Pull request description: This PR consists in small fixes in order to have a clean shutdown from the GUI. Tree-SHA512: a9c641f202bc810698c4a39d5c5a1f54e54bdab098c412d65418879e00764a9db9f38383813914d591e24e097e49f177942b2ae6c57bba05dcc095e8a1d0b8f4

fanquake added the GUI label Jan 29, 2019

promag mentioned this pull request Jan 29, 2019

gui: Add Open Wallet menu #15153

Merged

Sjors reviewed Jan 29, 2019

View reviewed changes

hebasto reviewed Jan 30, 2019

View reviewed changes

src/validationinterface.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2019-01-gui-shutdown branch 2 times, most recently from df785ea to 0b53bf9 Compare January 30, 2019 22:56

promag added 4 commits February 3, 2019 22:23

gui: Fix WalletController deletion

60e190c

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

gui: Expose BitcoinGUI::unsubscribeFromCoreSignals

07b9aad

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

promag force-pushed the 2019-01-gui-shutdown branch from 0b53bf9 to 0dd6a8c Compare February 3, 2019 22:24

laanwj merged commit 0dd6a8c into bitcoin:master Feb 4, 2019

promag deleted the 2019-01-gui-shutdown branch February 4, 2019 11:56

UdjinM6 mentioned this pull request Jan 3, 2020

AppInitMain should quit early and return false if shutdown was requested at some point dashpay/dash#3267

Merged

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Aug 11, 2021

partial bitcoin#15280 Check m_internals in UnregisterValidationInterface

f967582

kwvg pushed a commit to kwvg/dash that referenced this pull request Aug 12, 2021

partial bitcoin#15280 Check m_internals in UnregisterValidationInterface

b84dcec

UdjinM6 added a commit to dashpay/dash that referenced this pull request Aug 13, 2021

Merge pull request #4314 from kittywhiskers/blockfilters

607bd4b

merge bitcoin#15118, bitcoin#14172, bitcoin#15623, bitcoin#14121, partial bitcoin#13743, partial bitcoin#15280: block filters

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

	if (!model) {
	// Client model is being set to 0, this means shutdown() is about to be called.
	thread.quit();
	thread.wait();
	}

gui: Fix shutdown order #15280

gui: Fix shutdown order #15280

Uh oh!

Conversation

promag commented Jan 29, 2019

Uh oh!

Sjors left a comment

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 Jan 29, 2019 • 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!

jonasschnelli commented Jan 29, 2019

Uh oh!

Uh oh!

hebasto commented Jan 30, 2019

Uh oh!

Empact commented Jan 30, 2019

Uh oh!

promag commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 31, 2019

Conflicts

Uh oh!

Empact commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Feb 1, 2019

Uh oh!

laanwj commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

promag Jan 29, 2019 •

edited

Loading

promag commented Jan 30, 2019 •

edited

Loading

Empact commented Jan 31, 2019 •

edited

Loading