Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Fix shutdown order #15280

Merged
merged 4 commits into from Feb 4, 2019

Conversation

Projects
None yet
8 participants
@promag
Copy link
Member

commented Jan 29, 2019

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

@fanquake fanquake added the GUI label Jan 29, 2019

@promag promag referenced this pull request Jan 29, 2019

Merged

gui: Add Open Wallet menu #15153

@Sjors
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2019

Member

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

This comment has been minimized.

Copy link
@promag

promag Jan 29, 2019

Author Member

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

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2019

Member

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.

This comment has been minimized.

Copy link
@promag

promag Jan 29, 2019

Author Member

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.

@@ -95,6 +95,9 @@ class BitcoinGUI : public QMainWindow
*/
bool hasTrayIcon() const { return trayIcon; }

/** Disconnect core signals from GUI client */
void unsubscribeFromCoreSignals();

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2019

Member

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

This comment has been minimized.

Copy link
@promag

promag Jan 30, 2019

Author Member

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

This comment has been minimized.

Copy link
@Empact

Empact Jan 31, 2019

Member

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.

window->unsubscribeFromCoreSignals();
// Request node shutdown, which can interrupt long operations, like
// rescanning a wallet.
m_node.startShutdown();

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2019

Member

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?

This comment has been minimized.

Copy link
@promag

promag Jan 29, 2019

Author Member

Calling setClientModel(nullptr) hits:

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

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

utACK 870e35c

Show resolved Hide resolved src/validationinterface.cpp Outdated
@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

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

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

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 promag force-pushed the promag:2019-01-gui-shutdown branch 2 times, most recently from df785ea to 0b53bf9 Jan 30, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

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

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

  1. 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) {
  2. launch with -nowallet -rescan
  3. call loadwallet a_wallet_with_some_transactions in the console window
  4. close the application
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

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

promag added some commits Jan 29, 2019

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.

@promag promag force-pushed the promag:2019-01-gui-shutdown branch from 0b53bf9 to 0dd6a8c Feb 3, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

utACK 0dd6a8c

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Feb 4, 2019

Merge bitcoin#15280: gui: Fix shutdown order
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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@promag promag deleted the promag:2019-01-gui-shutdown branch Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.