gui: Try shutdown also on failure #13880

maflcko · 2018-08-04T22:45:34Z

The application would either quit or std::exit on failure and not even try a shutdown.

Note that bitcoind does a Interrupt and Shutdown in case the initialization fails or an exception is caught. So just do the same for the gui.

maflcko · 2018-08-04T22:46:16Z

Might fix #12337

laanwj · 2018-08-07T12:44:50Z

Concept ACK

promag

Concept ACK.

promag · 2018-08-07T22:35:27Z

src/qt/bitcoin.cpp

Add try/catch here instead? Otherwise should fix indentation above.

promag · 2018-08-07T22:36:18Z

src/qt/bitcoin.cpp

Could log something? Same below.

Any suggestions? Note that I'd rather not write to the debug.log, since that might be the cause of the exception.

I tend to agree that blanket-ignoring exceptions is very bad. Even printing to stderr would be better to at least have a potential chance of someone diagnosing of seeing the error!

The gui is not always started from the command line, so that would be missed often as well. But I guess I could add it.

jonasschnelli · 2018-08-22T14:07:31Z

Concept ACK

maflcko · 2018-08-22T20:17:03Z

Added a commit to print the exception (per common request)

ryanofsky

This PR is confusing and I think it is trying to do too many things. It would be easier to understand smaller PRs targeted at specific, reproducible problems.

ryanofsky · 2018-08-31T18:01:33Z

src/qt/bitcoin.cpp

I don't think it is right to call appShutdown() here because it should be covered by return quit(); below. quit should cause the first application loop to quit, which should trigger requestShutdown():

bitcoin/src/qt/bitcoin.cpp

Lines 718 to 720 in 59ecacf

app.exec();

app.requestShutdown();

app.exec();

which should trigger requestedShutdown():

bitcoin/src/qt/bitcoin.cpp

Lines 404 to 431 in 59ecacf

void BitcoinApplication::requestShutdown()

{

// Show a simple window indicating shutdown status

// Do this first as some of the steps may take some time below,

// for example the RPC console may still be executing a command.

shutdownWindow.reset(ShutdownWindow::showShutdownWindow(window));

qDebug() << __func__ << ": Requesting shutdown";

startThread();

window->hide();

window->setClientModel(0);

pollShutdownTimer->stop();

#ifdef ENABLE_WALLET

window->removeAllWallets();

for (WalletModel *walletModel : m_wallet_models) {

delete walletModel;

}

m_wallet_models.clear();

#endif

delete clientModel;

clientModel = 0;

m_node.startShutdown();

// Request shutdown from core thread

Q_EMIT requestedShutdown();

}

which should trigger BitcoinCore::shutdown:

bitcoin/src/qt/bitcoin.cpp

Line 379 in 59ecacf

connect(this, &BitcoinApplication::requestedShutdown, executor, &BitcoinCore::shutdown);

which already calls appShutdown():

bitcoin/src/qt/bitcoin.cpp

Lines 268 to 281 in 59ecacf

void BitcoinCore::shutdown()

{

try

{

qDebug() << __func__ << ": Running Shutdown in thread";

m_node.appShutdown();

qDebug() << __func__ << ": Shutdown finished";

Q_EMIT shutdownResult();

} catch (const std::exception& e) {

handleRunawayException(&e);

} catch (...) {

handleRunawayException(nullptr);

}

}

The shutdown process is definitely overcomplicated and should be simplified. I think a good direction would be to stop calling QApplication::quit during shutdown and instead just call it after. So Qt shutdown would just immediately follow bitcoin shutdown instead of being mingled with it.

ryanofsky · 2018-08-31T18:09:09Z

src/qt/bitcoin.cpp

Could call PrintExceptionContinue(nullptr) in these empty catches. boost::current_exception_diagnostic_information() could also be used to add more detail.

ryanofsky · 2018-08-31T18:23:59Z

src/qt/bitcoin.cpp

This doesn't seem like the right place to be calling appShutdown. The emit below is supposed to trigger BitcoinApplication::handleRunawayException:

bitcoin/src/qt/bitcoin.cpp

Line 377 in 59ecacf

connect(executor, &BitcoinCore::runawayException, this, &BitcoinApplication::handleRunawayException);

which is supposed to show an error dialog and then exit:

bitcoin/src/qt/bitcoin.cpp

Lines 523 to 527 in 59ecacf

void BitcoinApplication::handleRunawayException(const QString &message)

{

QMessageBox::critical(0, "Runaway exception", BitcoinGUI::tr("A fatal error occurred. Bitcoin can no longer continue safely and will quit.") + QString("\n\n") + message);

::exit(EXIT_FAILURE);

}

If you shut down in BitcoinCore::handleRunawayException, it seems like this is going to delay the dialog from showing up, and maybe even prevent it from being seen by the user. But if this is really an improvement, it would be good to have a comment explaining what the shutdown sequence is.

maflcko · 2018-09-04T20:11:00Z

I marked this up for grabs for someone to pick up and address the useful feedback by @ryanofsky

practicalswift · 2018-10-02T05:17:41Z

src/qt/bitcoin.cpp

e here shadows the function parameter in BitcoinCore::handleRunawayException(const std::exception *e).

DrahtBot · 2018-12-03T17:59:32Z

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

Conflicts

No conflicts as of last run.

fanquake · 2019-03-02T09:23:58Z

Closing, leaving Up for grabs.

maflcko added the GUI label Aug 4, 2018

promag reviewed Aug 7, 2018

View reviewed changes

ryanofsky reviewed Aug 31, 2018

View reviewed changes

maflcko added the Up for grabs label Sep 4, 2018

practicalswift reviewed Oct 2, 2018

View reviewed changes

DrahtBot added the Needs rebase label Dec 7, 2018

maflcko force-pushed the Mf1808-qtShutdownFail branch from fa9bda1 to 5263d45 Compare December 7, 2018 17:11

DrahtBot removed the Needs rebase label Dec 7, 2018

DrahtBot added the Needs rebase label Jan 14, 2019

MarcoFalke added 2 commits January 14, 2019 10:29

qt: Try shutdown also on failure

95d6b2b

qt: print exception when shutdown on failure throws

fd65a6d

maflcko force-pushed the Mf1808-qtShutdownFail branch from 5263d45 to fd65a6d Compare January 14, 2019 15:30

DrahtBot removed the Needs rebase label Jan 14, 2019

fanquake closed this Mar 2, 2019

maflcko deleted the Mf1808-qtShutdownFail branch March 2, 2019 14:39

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

maflcko removed the Up for grabs label Jul 24, 2025

	void BitcoinApplication::requestShutdown()
	{
	// Show a simple window indicating shutdown status
	// Do this first as some of the steps may take some time below,
	// for example the RPC console may still be executing a command.
	shutdownWindow.reset(ShutdownWindow::showShutdownWindow(window));

	qDebug() << __func__ << ": Requesting shutdown";
	startThread();
	window->hide();
	window->setClientModel(0);
	pollShutdownTimer->stop();

	#ifdef ENABLE_WALLET
	window->removeAllWallets();
	for (WalletModel *walletModel : m_wallet_models) {
	delete walletModel;
	}
	m_wallet_models.clear();
	#endif
	delete clientModel;
	clientModel = 0;

	m_node.startShutdown();

	// Request shutdown from core thread
	Q_EMIT requestedShutdown();
	}

	void BitcoinCore::shutdown()
	{
	try
	{
	qDebug() << __func__ << ": Running Shutdown in thread";
	m_node.appShutdown();
	qDebug() << __func__ << ": Shutdown finished";
	Q_EMIT shutdownResult();
	} catch (const std::exception& e) {
	handleRunawayException(&e);
	} catch (...) {
	handleRunawayException(nullptr);
	}
	}

	void BitcoinApplication::handleRunawayException(const QString &message)
	{
	QMessageBox::critical(0, "Runaway exception", BitcoinGUI::tr("A fatal error occurred. Bitcoin can no longer continue safely and will quit.") + QString("\n\n") + message);
	::exit(EXIT_FAILURE);
	}

gui: Try shutdown also on failure #13880

gui: Try shutdown also on failure #13880

Uh oh!

Conversation

maflcko commented Aug 4, 2018

Uh oh!

maflcko commented Aug 4, 2018

Uh oh!

laanwj commented Aug 7, 2018

Uh oh!

promag 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!

jonasschnelli commented Aug 22, 2018

Uh oh!

maflcko commented Aug 22, 2018

Uh oh!

ryanofsky 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!

maflcko commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake commented Mar 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Dec 3, 2018 •

edited

Loading