gui: Try shutdown also on failure

MarcoFalke 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.

MarcoFalke added the label GUI on Aug 4, 2018

MarcoFalke 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.

MarcoFalke commented at 0: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!

MarcoFalke 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: member

Concept ACK.

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

Concept ACK

MarcoFalke 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: member

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.

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

MarcoFalke 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: member

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

MarcoFalke 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

MarcoFalke 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

MarcoFalke deleted the branch on Mar 2, 2019

MarcoFalke locked this on Dec 16, 2021

gui: Try shutdown also on failure #13880

Conflicts