Do not block GUI thread in RPCConsole by hebasto · Pull Request #59 · bitcoin-core/gui

hebasto · 2020-08-14T10:32:02Z

On master (b4d0366) 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

hebasto · 2020-08-26T18:00:07Z

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

hebasto · 2020-08-26T18:02:24Z

@MarcoFalke Maybe add "Bug" label?

src/qt/bitcoingui.h

promag · 2020-09-25T12:05:40Z

src/qt/bitcoingui.cpp


+void BitcoinGUI::clearGUI()
+{
+    trayIconMenu->clear();


Why clear here?

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

How about trayIconMenu->setEnabled(false) instead?

hebasto · 2020-09-27T15:05:25Z

Updated 5549c45 -> 38efc5a (pr59.02 -> pr59.03, diff):

addressed @promag's comment:

hideAll?

jonasschnelli · 2020-10-23T11:09:04Z

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 · 2020-10-24T21:07:54Z

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

promag · 2020-10-25T23:25:24Z

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 · 2020-10-28T14:29:45Z

@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 · 2020-10-28T19:45:17Z

@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

ACK 38efc5a, 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.

hebasto · 2021-03-20T00:17:02Z

Rebased 38efc5a -> 84a9a7c (pr59.03 -> pr59.04) due to the conflicts with #115 and bitcoin/bitcoin#21328.

38eb37c qt, rpc: Do not accept command while executing another one (Hennadii Stepanov) 0c32b9c qt, rpc: Accept stop RPC even another command is executing (Hennadii Stepanov) ccf7902 qt, rpc, refactor: Return early in RPCConsole::on_lineEdit_returnPressed (Hennadii Stepanov) 5b9c8c9 qt, rpc: Add "Executing…" message (Hennadii Stepanov) Pull request description: On master (3f512f3) it is possible to enter another command while the current command is still being executed. That makes a mess in the output. With this PR: ![Screenshot from 2020-10-29 20-48-55](https://user-images.githubusercontent.com/32963518/97619690-329c0880-1a29-11eb-9f5b-6ae3c02c13b2.png) Some previous context: #59 (comment) --- It is still possible to enter and execute the `stop` command any time. ACKs for top commit: jarolrod: ACK 38eb37c promag: Tested ACK 38eb37c. Tree-SHA512: 2b37a4b6838bf586b1b5c878192106721f713caeb6252514a6540356aab898986396e0777e73891d331b1be797a4926c20d3f9f38ba2c984ea90d55b0c34f664

…g another one 38eb37c qt, rpc: Do not accept command while executing another one (Hennadii Stepanov) 0c32b9c qt, rpc: Accept stop RPC even another command is executing (Hennadii Stepanov) ccf7902 qt, rpc, refactor: Return early in RPCConsole::on_lineEdit_returnPressed (Hennadii Stepanov) 5b9c8c9 qt, rpc: Add "Executing…" message (Hennadii Stepanov) Pull request description: On master (3f512f3) it is possible to enter another command while the current command is still being executed. That makes a mess in the output. With this PR: ![Screenshot from 2020-10-29 20-48-55](https://user-images.githubusercontent.com/32963518/97619690-329c0880-1a29-11eb-9f5b-6ae3c02c13b2.png) Some previous context: bitcoin-core/gui#59 (comment) --- It is still possible to enter and execute the `stop` command any time. ACKs for top commit: jarolrod: ACK 38eb37c promag: Tested ACK 38eb37c. Tree-SHA512: 2b37a4b6838bf586b1b5c878192106721f713caeb6252514a6540356aab898986396e0777e73891d331b1be797a4926c20d3f9f38ba2c984ea90d55b0c34f664

451ca24 qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot (Hennadii Stepanov) f3a17bb qt: Do not exit and re-enter main event loop during shutdown (Hennadii Stepanov) b4e0d2c qt, refactor: Allocate SendConfirmationDialog instances on heap (Hennadii Stepanov) 332dea2 qt, refactor: Keep HelpMessageDialog in the main event loop (Hennadii Stepanov) c8bae37 qt, refactor: Keep PSBTOperationsDialog in the main event loop (Hennadii Stepanov) 7fa91e8 qt, refactor: Keep AskPassphraseDialog in the main event loop (Hennadii Stepanov) 6f6fde3 qt, refactor: Keep EditAddressDialog in the main event loop (Hennadii Stepanov) 59f7ba4 qt, refactor: Keep CoinControlDialog in the main event loop (Hennadii Stepanov) 7830cd0 qt, refactor: Keep OptionsDialog in the main event loop (Hennadii Stepanov) 13f6188 qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose (Hennadii Stepanov) Pull request description: On master (1ef34ee) during shutdown `QApplication` exits the main event loop, then re-enter again. This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for #59. Also, blocking [`QDialog::exec()`](https://doc.qt.io/qt-5/qdialog.html#exec) calls are replaced with safer [`QDialog::show()`](https://doc.qt.io/qt-5/qwidget.html#show), except for `SendConfirmationDialog` as that change is not trivial (marked as TODO). The [`QDialog::open()`](https://doc.qt.io/qt-5/qdialog.html#open) was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent. This PR does not change behavior, and all touched dialogs are still application modal. As a follow up, a design research could suggest to make some dialogs window modal. NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine. ACKs for top commit: laanwj: Code review and lighly tested ACK 451ca24 promag: ACK 451ca24, just changed signal to `quitRequested`. Tree-SHA512: ef01ab6ed803b202e776019a4e1f592e816f7bc786e00574b25a0bf16be2374ddf9db21f0a26da08700df7ef0ab9e879550df46dcfe3b6d940f5ed02ca5f8447

hebasto · 2021-09-30T13:29:08Z

#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 mentioned this pull request Aug 14, 2020

qt: Do not block GUI thread in RPCConsole bitcoin/bitcoin#17659

Closed

DrahtBot added the Needs rebase label Aug 26, 2020

hebasto force-pushed the 200814-rpc branch from 47912c5 to 5549c45 Compare August 26, 2020 17:58

maflcko added the Bug Something isn't working label Aug 26, 2020

DrahtBot removed the Needs rebase label Aug 26, 2020

promag reviewed Sep 25, 2020

View reviewed changes

hebasto force-pushed the 200814-rpc branch from 5549c45 to 38efc5a Compare September 27, 2020 15:04

hebasto mentioned this pull request Oct 28, 2020

generateto{address|descriptor} RPCs freeze the GUI #122

Open

hebasto mentioned this pull request Oct 28, 2020

rpc: Do not accept command while executing another one #123

Merged

jarolrod reviewed Jan 18, 2021

View reviewed changes

DrahtBot added the Needs rebase label Mar 19, 2021

hebasto added 5 commits March 20, 2021 01:48

qt: Add BitcoinGUI::hideAll()

88fc261

qt: Use BitcoinGUI::hideAll()

7a1d449

qt: Make requestShutdown() a slot

753103d

qt: Factor out requestNodeShutdown() slot

9af2549

qt: Do not block GUI thread in RPCConsole

84a9a7c

hebasto force-pushed the 200814-rpc branch from 38efc5a to 84a9a7c Compare March 20, 2021 00:13

DrahtBot removed the Needs rebase label Mar 20, 2021

hebasto marked this pull request as draft March 23, 2021 13:06

hebasto mentioned this pull request May 16, 2021

Do not exit and re-enter main event loop during shutdown #336

Merged

hebasto closed this Sep 30, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Sep 30, 2022

Conversation

hebasto commented Aug 14, 2020

Uh oh!

hebasto commented Aug 26, 2020

Uh oh!

hebasto commented Aug 26, 2020

Uh oh!

Uh oh!

promag Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 27, 2020

Choose a reason for hiding this comment

Uh oh!

promag Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 27, 2020

Uh oh!

jonasschnelli commented Oct 23, 2020

Uh oh!

luke-jr commented Oct 24, 2020

Uh oh!

promag commented Oct 25, 2020

Uh oh!

hebasto commented Oct 28, 2020

Uh oh!

hebasto commented Oct 28, 2020

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 20, 2021

Uh oh!

hebasto commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants