Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217)

LeandroRocha84 commented at 7:28 am on July 16, 2018: none

This pull request contains a fix for the issue #13217, where bitcoin-qt becomes unresponsive during shutdown after a long running call (like gettxoutsetinfo) in the QT console. The problem occurs because, during shutdown, waiting for the RPCExecutor thread to finish happens in the main thread.

fanquake added the label GUI on Jul 16, 2018

LeandroRocha84 commented at 5:27 pm on July 16, 2018: none

There’s also the possibility of emitting the shutdown signal from RPCExecutor destructor so we don’t need a wrapper - that simplify the solution. I created the PR with the wrapper solution because it might be considered unsafe to emit signals from a destructor in a multi-threaded application. Would appreciate any input on this and I’ll update the PR accordingly.

in src/qt/rpcconsole.cpp:1029 in debf7dbeb2 outdated

1029 
1030     // On stopExecutor signal
1031     // - quit the Qt event loop in the execution thread
1032-    connect(this, SIGNAL(stopExecutor()), &thread, SLOT(quit()));
1033+    connect(this, SIGNAL(stopExecutor()), executorWrapper, SLOT(shutdown()));
1034+    //connect(this, SIGNAL(stopExecutor()), &thread, SLOT(quit()));

promag commented at 8:44 pm on July 16, 2018:

Remove commented code?

promag commented at 8:59 pm on July 16, 2018: member

Indeed looks like unnecessary complexity. IIUC the main even loop blocks when joining the executor thread, so consider a different approach instead?

0QEventLoop loop;
1connect(&thread, SIGNAL(finished()), &loop, SLOT(quit()));
2if (!thread.isFinished()) loop.exec();

LeandroRocha84 force-pushed on Jul 17, 2018

LeandroRocha84 commented at 4:08 am on July 17, 2018: none

Thanks for the suggestion @promag I’ve updated the PR to remove the unnecessary complexity. Aside from your suggested code, I’ve made a small change to bitcoingui.cpp so that the UI clientModel is not cleared until the RPCExecutor thread joins. Otherwise, network events, like a new connection, that trigger a UI update would raise exceptions.

fanquake requested review from jonasschnelli on Jul 17, 2018

in src/qt/rpcconsole.cpp:693 in 3844ba3d10 outdated

688@@ -688,8 +689,10 @@ void RPCConsole::setClientModel(ClientModel *model)
689     if (!model) {
690         // Client model is being set to 0, this means shutdown() is about to be called.
691         // Make sure we clean up the executor thread
692+        QEventLoop loop;
693+        connect(&thread, SIGNAL(finished()), &loop, SLOT(quit()));

promag commented at 2:58 pm on July 17, 2018:

Sorry, could use new connect syntax?

0connect(&thread, &QThread::finished, &loop, &QEventLoop::quit)

LeandroRocha84 commented at 6:49 pm on July 18, 2018:

Sure thing. That’s done. And rebased.

LeandroRocha84 force-pushed on Jul 18, 2018

in src/qt/bitcoingui.cpp:473 in 086d88e3e9 outdated

469@@ -470,13 +470,13 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
470         }
471 #endif // ENABLE_WALLET
472         unitDisplayControl->setOptionsModel(_clientModel->getOptionsModel());
473-        
474+

promag commented at 5:15 pm on July 26, 2018:

Rebase to remove this change.

LeandroRocha84 commented at 3:00 am on July 27, 2018:

Done

in src/qt/bitcoingui.cpp:553 in 086d88e3e9 outdated

436@@ -437,9 +437,9 @@ void BitcoinGUI::createToolBars()
437 
438 void BitcoinGUI::setClientModel(ClientModel *_clientModel)
439 {
440-    this->clientModel = _clientModel;
441     if(_clientModel)
442     {
443+        this->clientModel = _clientModel;

promag commented at 5:15 pm on July 26, 2018:

Why this change? Should update this->clientModel event if _clientModel is null.

LeandroRocha84 commented at 2:52 am on July 27, 2018:

Because now the main thread is not blocked, network events can trigger a GUI update. If the clientModel is set to 0 or null at the time, the application will crash. Example: When number of connections change, BitcoinGUI::updateNetworkState() gets invoked and this method assumes that clientModel is valid. With this change BitcoinGUI clientModel is cleared only after the RPCExecutor thread joins and shutdown resumes in the main thread.

promag commented at 10:40 am on April 20, 2019:

BitcoinGUI::updateNetworkState() gets invoked and this method assumes that clientModel is valid.

Then those need to be fixed IMO. With your change calling setClientModel(nullptr) on the BitcoinGUI instance is a noop.

in src/qt/bitcoingui.cpp:479 in 086d88e3e9 outdated

476         if(optionsModel)
477         {
478             // be aware of the tray icon disable state change reported by the OptionsModel object.
479             connect(optionsModel,SIGNAL(hideTrayIconChanged(bool)),this,SLOT(setTrayIconVisible(bool)));
480-        
481+

promag commented at 5:16 pm on July 26, 2018:

Same as above.

LeandroRocha84 commented at 3:00 am on July 27, 2018:

Done!

LeandroRocha84 force-pushed on Jul 27, 2018

LeandroRocha84 commented at 5:45 am on September 1, 2018: none

Hi all, please let me know if there’s anything that needs to be done to have this fix merged. Thanks

in src/qt/rpcconsole.cpp:695 in 9255963890 outdated

688@@ -688,8 +689,10 @@ void RPCConsole::setClientModel(ClientModel *model)
689     if (!model) {
690         // Client model is being set to 0, this means shutdown() is about to be called.
691         // Make sure we clean up the executor thread
692+        QEventLoop loop;
693+        connect(&thread, &QThread::finished, &loop, &QEventLoop::quit);
694         Q_EMIT stopExecutor();
695-        thread.wait();
696+        if (!thread.isFinished()) loop.exec();

hebasto commented at 7:37 pm on September 22, 2018:

You’re implicitly using QueuedConnection here: https://github.com/bitcoin/bitcoin/blob/9255963890f07f93646cc32703da935dcb24d454/src/qt/rpcconsole.cpp#L693 Therefore, the thread.isFinished() is called immediately after the stopExecutor() signal is emitted. If the thread is idle the value of thread.isFinished() may be unpredictable, IMO.

LeandroRocha84 commented at 6:47 am on September 30, 2018:

If I understood your concern correctly, you’re afraid that the thread.isFinished() result may be unpredictable and as a result, the QEventLoop may never quit since it is tied to the thread finished() signal.

I don’t see a problem considering how the QThread lifecycle works. As per documentation, the order is:

Thread quit() or exit() is called
As soon as exec completes, isFinished starts to return true
The finished() signal is emitted

As a result we should consistently get the loop.exec before the thread finished signal is emitted. Also, if thread.isFinished return true, the code in the if statement is not executed and shutdown continues normally.

Please let me know if I misunderstood your concern. Thanks,

hebasto commented at 1:19 pm on October 21, 2018:

~if (!thread.isFinished()) loop.exec(); is not atomic so the wrong order (loop.quit() then loop.exec()) is possible.~ See: #13674 (review)

LeandroRocha84 commented at 7:38 am on November 6, 2018:

As you noticed, we’re using QueuedConnection implicitly here. The EventLoop will only receive the thread finished event after we call exec(). I ran multiple tests locally to confirm that. Even when the thread finishes right away and we insert some long running process before the exec, it still exits successfully. That would only be a problem if we were using Qt::DirectConnection. @promag , do you have anything to add to this comment? (as you suggested that snippet of code)

hebasto commented at 10:34 am on December 3, 2018:

Agree: forcing event dispatching by means of a local event loop works as expected.

DrahtBot commented at 0:08 am on October 24, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15894 (Remove duplicated “Error: " prefix in logs by hebasto)
#9849 (Qt: Network Watch tool by luke-jr)

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.

in src/qt/bitcoingui.cpp:495 in 9255963890 outdated

487@@ -488,8 +488,11 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
488             // Disable context menu on tray icon
489             trayIconMenu->clear();
490         }
491-        // Propagate cleared model to child objects
492+        // Propagate cleared model to RPC console
493         rpcConsole->setClientModel(nullptr);
494+
495+        // Once RPC console is cleared, propagate to other child objects
496+        this->clientModel = 0;

hebasto commented at 10:36 am on December 3, 2018:

0this->clientModel = nullptr;

LeandroRocha84 commented at 8:02 am on December 5, 2018:

Done. Changed to nullptr. I had it set to 0 before because it is the current behavior in master. BitcoinApplication::requestShutdown() in bitcoin.cpp invokes window->setClientModel(0)

hebasto commented at 8:22 am on December 5, 2018:

Developer Notes - Coding Style (General):

When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.

hebasto changes_requested

hebasto commented at 8:25 am on December 5, 2018: member

tACK 18850903f803bba58d110d3994df18969992f383 (on Linux) Would you squash commits?

LeandroRocha84 force-pushed on Dec 5, 2018

LeandroRocha84 commented at 7:06 am on December 6, 2018: none

All comments addressed. Commits squashed. Please let me know if there’s anything.

hebasto commented at 7:22 pm on December 6, 2018: member

re-tACK abff36969da9998e36c72f2df7faf68564f0c8db (Linux, macOS 10.13.6)

DrahtBot added the label Needs rebase on Jan 17, 2019

jonasschnelli commented at 7:54 am on April 16, 2019: contributor

@LeandroRocha84 sorry for keeping this up so long… can you rebase once and then we try to get this merged?

LeandroRocha84 commented at 6:18 am on April 17, 2019: none

Thanks @jonasschnelli I’m working on solving the conflicts after the rebase. The order change in fd6d499 breaks the solution in this PR as it makes the QEventLoop.exec returns right away. I’ve confirmed that the bug this PR intends to solve is still reproducible; the shutdown screen is frozen/blocked when we have a long running RPC command. I’m going to spend some time this weekend to find out how exactly the order change I linked affects the QEventLoop.

Adding @promag as he might have some insight.

LeandroRocha84 force-pushed on Apr 19, 2019

LeandroRocha84 force-pushed on Apr 20, 2019

LeandroRocha84 commented at 6:18 am on April 20, 2019: none

Conflicts resolved, rebase completed. I’ve tested the fix on linux. @jonasschnelli please let me know if there’s anything else I can do to get this pull request completed. Thank you.

in src/qt/rpcconsole.cpp:693 in a302c992ba outdated

686@@ -686,8 +687,11 @@ void RPCConsole::setClientModel(ClientModel *model)
687     }
688     if (!model) {
689         // Client model is being set to 0, this means shutdown() is about to be called.
690+        QEventLoop loop;
691+        connect(&thread, &QThread::finished, &loop, &QEventLoop::quit);
692         thread.quit();
693-        thread.wait();
694+        if (!thread.isFinished())

promag commented at 10:42 am on April 20, 2019:

Add braces {.

in src/qt/bitcoingui.cpp:1317 in a302c992ba outdated

1313@@ -1311,7 +1314,8 @@ void BitcoinGUI::detectShutdown()
1314     {
1315         if(rpcConsole)
1316             rpcConsole->hide();
1317-        qApp->quit();
1318+        if(!closeEventRegistered)

promag commented at 10:42 am on April 20, 2019:

Should be

0if (!closeEventRegistered) qApp->quit();

Or

0if (!closeEventRegistered) {
1    qApp->quit();
2}

LeandroRocha84 commented at 3:18 pm on April 20, 2019:

Done.

in src/qt/bitcoingui.h:164 in a302c992ba outdated

160@@ -161,6 +161,7 @@ class BitcoinGUI : public QMainWindow
161     RPCConsole* rpcConsole = nullptr;
162     HelpMessageDialog* helpMessageDialog = nullptr;
163     ModalOverlay* modalOverlay = nullptr;
164+    bool closeEventRegistered = false;

promag commented at 10:44 am on April 20, 2019:

This is used to prevent calling qApp->quit() more than once. Is this a real problem - could this be removed?

LeandroRocha84 commented at 3:34 pm on April 20, 2019:

That’s right. This is needed since the reordering in fd6d499. The second call to aApp-quit() would make the QEventLoop exit before the RPC executor thread finishes causing the application to crash. I’ve made sure that closing bitcoin QT by typing in ‘stop’ in the console still works as intended when this line was introduced in 9a2305a

promag changes_requested

LeandroRocha84 force-pushed on Apr 20, 2019

DrahtBot removed the label Needs rebase on Apr 22, 2019

in src/qt/rpcconsole.cpp:693 in ba230e4299 outdated

686@@ -686,8 +687,12 @@ void RPCConsole::setClientModel(ClientModel *model)
687     }
688     if (!model) {
689         // Client model is being set to 0, this means shutdown() is about to be called.
690+        QEventLoop loop;
691+        connect(&thread, &QThread::finished, &loop, &QEventLoop::quit);
692         thread.quit();
693-        thread.wait();
694+        if (!thread.isFinished()) {

promag commented at 7:39 pm on April 24, 2019:

Between thread.isFinished() and loop.exec() the thread can terminate and then loop won’t quit.

LeandroRocha84 commented at 2:40 am on April 25, 2019:

As we’re implicitly using QueuedConnection, the thread finished signal won’t be produced until after loop.exec() is called, that’s my understanding based on the docs. Please let me know if you think I’m wrong here - I’ve tested this a lot, including with varying delays between thread.isFinished() and loop.exec() Another comment I wrote on this PR in a thread with @hebasto : #13674 (review)

Let me know whether you still think this is a problem.

promag commented at 8:29 am on April 25, 2019:

You are correct, I’ve verified QThread::finished signal is emitted from a different thread which defers the QEventLoop::quit execution.

However, considering #15313, I’m not sure if nested event loops should be avoided. Sorry for suggesting this approach but #15313 raised some concerns in this regard.

An alternative is to expose a signal RPCConsole::finished to BitcoinGUI.

LeandroRocha84 commented at 9:11 pm on May 5, 2019:

I see the point. That’s done, I’ve exposed the thread finished signal to BitcoinGUI Can you please review? Thanks,

LeandroRocha84 force-pushed on May 5, 2019

Allow GUI to remain responsive during shutdown when there's a long running RPC call 0636f38ad1

LeandroRocha84 force-pushed on May 5, 2019

DrahtBot added the label Needs rebase on Jun 25, 2019

DrahtBot commented at 11:56 am on June 25, 2019: member

DrahtBot commented at 1:02 pm on September 30, 2019: member

DrahtBot added the label Up for grabs on Sep 30, 2019

DrahtBot closed this on Sep 30, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke removed the label Up for grabs on Dec 3, 2019

DrahtBot locked this on Dec 16, 2021

Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) #13674

Conflicts