rpc: Fix gui shutdown when waitfor* cmds are called from RPC console

hebasto commented at 6:44 pm on March 27, 2020: member

On master (7eed413e72a236b6f1475a198f7063fd24929e23), if the GUI has been started with-server=1, bitcoin-qt hangs on shutdown during calling any of the waitfor* commands in the GUI RPC console.

This PR suggests minimal changes to fix this bug.

Fix #17495

hebasto commented at 7:06 pm on March 27, 2020: member

While the Travis-to-GitHub connection is broken, here is Travis’ build of this PR.

DrahtBot added the label GUI on Mar 27, 2020

DrahtBot added the label RPC/REST/ZMQ on Mar 27, 2020

DrahtBot commented at 11:42 pm on March 27, 2020: member

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

Conflicts

No conflicts as of last run.

promag commented at 0:25 am on March 30, 2020: member

Concept ACK. Have you considered doing this in BitcoinApplication::requestShutdown or even between app.exec() and app.requestShutdown() in qt/bitcoin.cpp?

hebasto commented at 7:18 am on March 30, 2020: member

@promag

Have you considered doing this in BitcoinApplication::requestShutdown or even between app.exec() and app.requestShutdown() in qt/bitcoin.cpp?

Yes, I have. InterruptRPC(); StopRPC(); could be placed just before the following line https://github.com/bitcoin/bitcoin/blob/5f9cd62f33fb4d440173b9c376cadf4887e81e9d/src/qt/bitcoin.cpp#L316

What are benefits of such approach while it requires to add #include <rpc/server.h> to qt/bitcoin.cpp?

Also, please note, that #17659 suggests to streamline the bitcoin-qt shutdown routine and leave the only QApplication::exec() call.

in src/qt/rpcconsole.cpp:692 in 29f5e02601 outdated

686@@ -687,7 +687,12 @@ void RPCConsole::setClientModel(ClientModel *model)
687         startExecutor();
688     }
689     if (!model) {
690-        // Client model is being set to 0, this means shutdown() is about to be called.
691+        // Client model is being set to nullptr, this means shutdown() is about to be called.
692+        if (gArgs.GetBoolArg("-server", false)) {
693+            InterruptRPC();

jonasschnelli commented at 10:15 am on April 8, 2020:

This feels to be the wrong place to call shutdown/init functions. Shouldn’t this be called through bitcoin.cpp?

hebasto commented at 3:26 pm on April 8, 2020:

Done.

jonasschnelli commented at 10:15 am on April 8, 2020: contributor

Concept ACK

hebasto force-pushed on Apr 8, 2020

hebasto commented at 3:25 pm on April 8, 2020: member

Updated 29f5e02601730298020827c837db23ac6c7a8010 -> 368530de424fcb73541b089ec555a44abe475a6d (pr18452.01 -> pr18452.02, diff):

addressed @promag’s comment

Concept ACK. Have you considered doing this in BitcoinApplication::requestShutdown or even between app.exec() and app.requestShutdown() in qt/bitcoin.cpp?

addressed @jonasschnelli’s comment

This feels to be the wrong place to call shutdown/init functions. Shouldn’t this be called through bitcoin.cpp?

in src/qt/bitcoin.cpp:315 in 368530de42 outdated

311@@ -311,6 +312,11 @@ void BitcoinApplication::requestShutdown()
312     // Request node shutdown, which can interrupt long operations, like
313     // rescanning a wallet.
314     m_node.startShutdown();
315+    // Stop RPC for clean shutdown if waitfor* commands are executed.

promag commented at 10:50 am on April 10, 2020:

I think you could move this to NodeImpl::startShutdown?

hebasto commented at 2:11 pm on April 10, 2020:

There are other NodeImpl::startShutdown() call sites that are irrelevant to the problem: https://github.com/bitcoin/bitcoin/blob/2b9a4a13324ac3f0adf712d01fb8f2ba914cdc8f/src/qt/splashscreen.cpp#L141

and

https://github.com/bitcoin/bitcoin/blob/2b9a4a13324ac3f0adf712d01fb8f2ba914cdc8f/src/qt/splashscreen.cpp#L223

promag commented at 2:24 pm on April 10, 2020:

I did check those and TBH I think it’s still fine.

hebasto commented at 2:31 pm on April 10, 2020:

I did check those and TBH I think it’s still fine.

To be clear, what exactly is “still fine” in your opinion: pr code or your suggestion to move to the NodeImpl ?

hebasto commented at 5:33 pm on April 10, 2020:

Implemented.

promag commented at 11:03 am on April 10, 2020: member

Code review ACK 368530de424fcb73541b089ec555a44abe475a6d, the approach looks better.

hebasto force-pushed on Apr 10, 2020

hebasto commented at 5:32 pm on April 10, 2020: member

Updated 368530de424fcb73541b089ec555a44abe475a6d -> 59b55ccf9aae2f75e86245434be26e1f1a729659 (pr18452.02 -> pr18452.03, diff):

addressed @promag’s comment

I think you could move this to NodeImpl::startShutdown?

promag commented at 11:30 am on April 11, 2020: member

Tested ACK 59b55ccf9aae2f75e86245434be26e1f1a729659.

in src/rpc/server.cpp:311 in 59b55ccf9a outdated

306+    assert(!g_rpc_running);
307+    if (g_rpc_stopped) return;
308     LogPrint(BCLog::RPC, "Stopping RPC\n");
309     deadlineTimers.clear();
310     DeleteAuthCookie();
311+    g_rpc_stopped = true;

luke-jr commented at 0:12 am on April 23, 2020:

Race?

hebasto commented at 0:09 am on April 28, 2020:

Updated.

hebasto force-pushed on Apr 28, 2020

hebasto commented at 0:09 am on April 28, 2020: member

Updated 59b55ccf9aae2f75e86245434be26e1f1a729659 -> 2d7b6bc6c9c49ac95c7aa97d9f753aec97456e0b (pr18452.03 -> pr18452.04, diff):

addressed @luke-jr’s comment

Race?

DrahtBot added the label Needs rebase on May 13, 2020

qt: Fix shutdown when waitfor* cmds are called from RPC console da73f1513a

hebasto force-pushed on May 13, 2020

hebasto commented at 1:16 pm on May 13, 2020: member

Rebased 2d7b6bc6c9c49ac95c7aa97d9f753aec97456e0b -> da73f1513a637a9f347b64de66564d6cdb2541f8 (pr18452.04 -> pr18452.05) due to the conflict with #18814.

DrahtBot removed the label Needs rebase on May 13, 2020

jonasschnelli commented at 1:49 pm on May 29, 2020: contributor

utACK da73f1513a637a9f347b64de66564d6cdb2541f8

jonasschnelli merged this on May 29, 2020

jonasschnelli closed this on May 29, 2020

hebasto deleted the branch on May 29, 2020

MarcoFalke removed the label GUI on May 29, 2020

MarcoFalke renamed this:
~~qt: Fix shutdown when waitfor* cmds are called from RPC console~~
rpc: Fix gui shutdown when waitfor* cmds are called from RPC console
on May 29, 2020

sidhujag referenced this in commit ff0692d4f3 on May 31, 2020

luke-jr referenced this in commit e5a935c7db on Jun 9, 2020

deadalnix referenced this in commit caf502a98a on Feb 24, 2021

DrahtBot locked this on Feb 15, 2022

rpc: Fix gui shutdown when waitfor* cmds are called from RPC console #18452

Conflicts