Do not block GUI thread in RPCConsole

hebasto commented at 10:32 am on August 14, 2020: member

Ported from https://github.com/bitcoin/bitcoin/pull/17659.

On master (b4d0366b47dd9b8fe29cc9a100dcdf6ca1d3cabf) the GUI thread is blocked with QThread::wait() during bitcoin-qt shutdown routine. This causes unresponsive GUI if some commands are passed to the RPC console (#53) before shutdown initiating.

This PR:

removes blocking call and uses additional signal-to-slot connections.
makes bitcoin-qt shutdown routine more streamlined: the only QApplication::exec() is used in bitcoin-qt. Therefore, the main event loop never interrupts until shutdown.

Fix #53

DrahtBot added the label Needs rebase on Aug 26, 2020

hebasto force-pushed on Aug 26, 2020

hebasto commented at 6:00 pm on August 26, 2020: member

Rebased 47912c505eebc79c849cf14fcf9d2654caa0945a -> 5549c453b1817084dd9b5f463ecd4d8277b973e8 (pr59.01 -> pr59.02) due to the conflict with #35.

hebasto commented at 6:02 pm on August 26, 2020: member

@MarcoFalke Maybe add “Bug” label?

MarcoFalke added the label Bug on Aug 26, 2020

DrahtBot removed the label Needs rebase on Aug 26, 2020

in src/qt/bitcoingui.h:325 in ecd774ff93 outdated

319@@ -320,6 +320,9 @@ public Q_SLOTS:
320     void setTrayIconVisible(bool);
321 
322     void showModalOverlay();
323+
324+    /** Hide all windows and tray icon. */
325+    void clearGUI();

promag commented at 11:55 am on September 25, 2020:

ecd774ff933472e701987d689445f61444225db6

hideAll?

hebasto commented at 3:05 pm on September 27, 2020:

Thanks! Updated.

in src/qt/bitcoingui.cpp:1389 in ecd774ff93 outdated

1394@@ -1395,6 +1395,14 @@ void BitcoinGUI::showModalOverlay()
1395         modalOverlay->toggleVisibility();
1396 }
1397 
1398+void BitcoinGUI::clearGUI()
1399+{
1400+    trayIconMenu->clear();

promag commented at 12:05 pm on September 25, 2020:

Why clear here?

hebasto commented at 2:43 pm on September 27, 2020:

The trayIconMenu contains some QAction instances. I think we do not need any chance to activate any of them during shutdown.

promag commented at 1:54 pm on September 29, 2021:

How about trayIconMenu->setEnabled(false) instead?

hebasto force-pushed on Sep 27, 2020

hebasto commented at 3:05 pm on September 27, 2020: member

Updated 5549c453b1817084dd9b5f463ecd4d8277b973e8 -> 38efc5a11294ac56a9a098139813b94faf3979e9 (pr59.02 -> pr59.03, diff):

addressed @promag’s comment:

hideAll?

jonasschnelli commented at 11:09 am on October 23, 2020: contributor

Tested this a bit.

I can’t shutdown with this PR when I call gettxoutsetinfo in the RPC console
Calling generate(1000, getnewaddress()) still blocks the GUI thread

What I’d like to see is:

Keep the RPC console synchronous “single channeled”. If I call gettxoutsetinfo, regardless of the non-blocking GUI mode, it should not allow to enter a next command before the old has completed.
Things like “generate(1000, getnewaddress())” should not block the GUI and there should be an idle indicator (maybe just the text “executing…”) in the RPC console.

luke-jr commented at 9:07 pm on October 24, 2020: member

Seems like it would be even better to allow multiple RPC executions, and insert the return values in the applicable place :)

promag commented at 11:25 pm on October 25, 2020: contributor

Seems like it would be even better to allow multiple RPC executions, and insert the return values in the applicable place :)

Why? Seems a source of confusion.

hebasto commented at 2:29 pm on October 28, 2020: member

@jonasschnelli

Calling generate(1000, getnewaddress()) still blocks the GUI thread

I’ve made some debugging, and now I believe that this is another issue, which is uncorrelated to #53, and it has nothing to do with the RPCExecutor thread.

Not sure if #122 should be fixed at all, as it is a pretty edge case, and a possible fix could touch the code at least in three important places.

hebasto commented at 7:45 pm on October 28, 2020: member

@jonasschnelli

What I’d like to see is:

Keep the RPC console synchronous “single channeled”. If I call gettxoutsetinfo, regardless of the non-blocking GUI mode, it should not allow to enter a next command before the old has completed.

Things like “generate(1000, getnewaddress())” should not block the GUI and there should be an idle indicator (maybe just the text “executing…”) in the RPC console.

Mind looking into #123 ?

jarolrod commented at 11:05 pm on January 18, 2021: member

ACK 38efc5a11294ac56a9a098139813b94faf3979e9, Tested on macOS 11.1 with Qt 5.15.2

Confirming that I can replicate the bug described in #53 on master.

Confirming that this PR allows me to shutdown the GUI right after calling gettxoutsetinfo both in the Console Window and through bitcoin-cli gettxoutsetinfo after running the GUI with the -server option.

Confirming that I can replicate the issue laid out by @jonasschnelli comment about generate. There is no longer a generate command available in the Console Window, instead I replicated with bitcoin-cli -generate 10000 after running bitcoin-qt with -server. But, this bug is meant to be fixed by #123.

DrahtBot added the label Needs rebase on Mar 19, 2021

qt: Add BitcoinGUI::hideAll() 88fc261fc4

qt: Use BitcoinGUI::hideAll() 7a1d449f82

qt: Make requestShutdown() a slot 753103dd53

qt: Factor out requestNodeShutdown() slot 9af25492dc

qt: Do not block GUI thread in RPCConsole 84a9a7ce4d

hebasto force-pushed on Mar 20, 2021

hebasto commented at 0:17 am on March 20, 2021: member

Rebased 38efc5a11294ac56a9a098139813b94faf3979e9 -> 84a9a7ce4ddf6627afca42e6ed6fd8ead55503f9 (pr59.03 -> pr59.04) due to the conflicts with #115 and https://github.com/bitcoin/bitcoin/pull/21328.

DrahtBot removed the label Needs rebase on Mar 20, 2021

hebasto marked this as a draft on Mar 23, 2021

hebasto referenced this in commit 684e687d42 on Jun 1, 2021

sidhujag referenced this in commit f42b4dd719 on Jun 1, 2021

laanwj referenced this in commit 81e7748bc1 on Sep 30, 2021

hebasto commented at 1:29 pm on September 30, 2021: member

#53 (comment):

On the current master (bd40cd8) this bug is no longer reproducible. However, the blocking call QThread::wait() is still present in the RPCConsole::setClientModel function.

Tested on Linux Mint 20.2 (Qt 5.12.8) and on Windows 10 Pro 20H2 (MSVC build).

Closing for now.

Closing.

hebasto closed this on Sep 30, 2021

bitcoin-core locked this on Sep 30, 2022

Do not block GUI thread in RPCConsole #59