gui: Try shutdown also on failure #13880

maflcko commented at 10:45 PM on August 4, 2018: member

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 added the label GUI on Aug 4, 2018

maflcko commented at 10:46 PM on August 4, 2018: member

Might fix #12337

laanwj commented at 12:44 PM on August 7, 2018: member

Concept ACK

in src/qt/bitcoin.cpp:517 in fa4a10f4c6 outdated

 523 | @@ -513,10 +524,6 @@ void BitcoinApplication::initializeResult(bool success)
 524 |          QTimer::singleShot(100, paymentServer, SLOT(uiReady()));
 525 |  #endif
 526 |          pollShutdownTimer->start(200);
 527 | -    } else {
 528 | -        Q_EMIT splashFinished(window); // Make sure splash screen doesn't stick around during shutdown

promag commented at 10:35 PM on August 7, 2018:

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

in src/qt/bitcoin.cpp:152 in fa4a10f4c6 outdated

 250 | @@ -251,6 +251,10 @@ BitcoinCore::BitcoinCore(interfaces::Node& node) :
 251 |  void BitcoinCore::handleRunawayException(const std::exception *e)
 252 |  {
 253 |      PrintExceptionContinue(e, "Runaway exception");
 254 | +    try {
 255 | +        m_node.appShutdown();
 256 | +    } catch (...) {
 257 | +    }

promag commented at 10:36 PM on August 7, 2018:

Could log something? Same below.

maflcko commented at 12:32 AM on August 8, 2018:

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

laanwj commented at 3:34 PM on August 22, 2018:

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!

maflcko commented at 3:43 PM on August 22, 2018:

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.

promag commented at 10:37 PM on August 7, 2018: contributor

Concept ACK.

jonasschnelli commented at 2:07 PM on August 22, 2018: contributor

Concept ACK

maflcko commented at 8:17 PM on August 22, 2018: member

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

in src/qt/bitcoin.cpp:376 in fa9bda1141 outdated

 470 | @@ -465,8 +471,17 @@ void BitcoinApplication::initializeResult(bool success)
 471 |      qDebug() << __func__ << ": Initialization result: " << success;
 472 |      // Set exit result.
 473 |      returnValue = success ? EXIT_SUCCESS : EXIT_FAILURE;
 474 | -    if(success)
 475 | -    {
 476 | +    if (!success) {
 477 | +        try {
 478 | +            m_node.appShutdown();

ryanofsky commented at 6:01 PM on August 31, 2018:

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

https://github.com/bitcoin/bitcoin/blob/59ecacfc84af13e5a1608e7d970315d07dcb0269/src/qt/bitcoin.cpp#L718-L720

which should trigger requestedShutdown():

https://github.com/bitcoin/bitcoin/blob/59ecacfc84af13e5a1608e7d970315d07dcb0269/src/qt/bitcoin.cpp#L404-L431

which should trigger BitcoinCore::shutdown:

https://github.com/bitcoin/bitcoin/blob/59ecacfc84af13e5a1608e7d970315d07dcb0269/src/qt/bitcoin.cpp#L379

which already calls appShutdown():

https://github.com/bitcoin/bitcoin/blob/59ecacfc84af13e5a1608e7d970315d07dcb0269/src/qt/bitcoin.cpp#L268-L281

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.

in src/qt/bitcoin.cpp:152 in fa9bda1141 outdated

 250 | @@ -251,6 +251,12 @@ BitcoinCore::BitcoinCore(interfaces::Node& node) :
 251 |  void BitcoinCore::handleRunawayException(const std::exception *e)
 252 |  {
 253 |      PrintExceptionContinue(e, "Runaway exception");
 254 | +    try {
 255 | +        m_node.appShutdown();
 256 | +    } catch (const std::exception& e) {
 257 | +        PrintExceptionContinue(&e, __func__);
 258 | +    } catch (...) {
 259 | +    }

ryanofsky commented at 6:09 PM on August 31, 2018:

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

in src/qt/bitcoin.cpp:148 in fa9bda1141 outdated

 250 | @@ -251,6 +251,12 @@ BitcoinCore::BitcoinCore(interfaces::Node& node) :
 251 |  void BitcoinCore::handleRunawayException(const std::exception *e)
 252 |  {
 253 |      PrintExceptionContinue(e, "Runaway exception");
 254 | +    try {
 255 | +        m_node.appShutdown();

ryanofsky commented at 6:23 PM on August 31, 2018:

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

https://github.com/bitcoin/bitcoin/blob/59ecacfc84af13e5a1608e7d970315d07dcb0269/src/qt/bitcoin.cpp#L377

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

https://github.com/bitcoin/bitcoin/blob/59ecacfc84af13e5a1608e7d970315d07dcb0269/src/qt/bitcoin.cpp#L523-L527

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.

ryanofsky commented at 6:33 PM on August 31, 2018: contributor

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.

maflcko added the label Up for grabs on Sep 4, 2018

maflcko commented at 8:11 PM on September 4, 2018: member

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

in src/qt/bitcoin.cpp:149 in fa9bda1141 outdated

 250 | @@ -251,6 +251,12 @@ BitcoinCore::BitcoinCore(interfaces::Node& node) :
 251 |  void BitcoinCore::handleRunawayException(const std::exception *e)
 252 |  {
 253 |      PrintExceptionContinue(e, "Runaway exception");
 254 | +    try {
 255 | +        m_node.appShutdown();
 256 | +    } catch (const std::exception& e) {

practicalswift commented at 5:17 AM on October 2, 2018:

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

DrahtBot commented at 5:59 PM on December 3, 2018: contributor

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Dec 7, 2018

maflcko force-pushed on Dec 7, 2018

DrahtBot removed the label Needs rebase on Dec 7, 2018

DrahtBot added the label Needs rebase on Jan 14, 2019

qt: Try shutdown also on failure 95d6b2ba1c

qt: print exception when shutdown on failure throws fd65a6de15

maflcko force-pushed on Jan 14, 2019

DrahtBot removed the label Needs rebase on Jan 14, 2019

fanquake commented at 9:23 AM on March 2, 2019: member

Closing, leaving Up for grabs.

fanquake closed this on Mar 2, 2019

maflcko deleted the branch on Mar 2, 2019

bitcoin locked this on Dec 16, 2021

maflcko removed the label Up for grabs on Jul 24, 2025