Skip to content

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

Merged
jonasschnelli merged 1 commit into
bitcoin:masterfrom
hebasto:20200327-waitfor
May 29, 2020
Merged

rpc: Fix gui shutdown when waitfor* cmds are called from RPC console#18452
jonasschnelli merged 1 commit into
bitcoin:masterfrom
hebasto:20200327-waitfor

Conversation

@hebasto

@hebasto hebasto commented Mar 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

This was referenced Mar 27, 2020
Closed
Closed
@hebasto

hebasto commented Mar 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@DrahtBot

DrahtBot commented Mar 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Conflicts

No conflicts as of last run.

promag
promag reviewed Mar 30, 2020
View reviewed changes

@promag promag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hebasto

hebasto commented Mar 30, 2020

Copy link
Copy Markdown
Member Author

@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

bitcoin/src/qt/bitcoin.cpp

Line 316 in 5f9cd62

window->setClientModel(nullptr);

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.

jonasschnelli
jonasschnelli reviewed Apr 8, 2020
View reviewed changes
Comment thread src/qt/rpcconsole.cpp Outdated
@jonasschnelli

jonasschnelli commented Apr 8, 2020

Copy link
Copy Markdown
Contributor

Concept ACK

@hebasto
hebasto force-pushed the 20200327-waitfor branch from 29f5e02 to 368530d Compare April 8, 2020 15:19
@hebasto

hebasto commented Apr 8, 2020

Copy link
Copy Markdown
Member Author

Updated 29f5e02 -> 368530d (pr18452.01 -> pr18452.02, diff):

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

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

promag
promag reviewed Apr 10, 2020
View reviewed changes

@promag promag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 368530d, the approach looks better.

Comment thread src/qt/bitcoin.cpp Outdated
@hebasto

hebasto commented Apr 10, 2020

Copy link
Copy Markdown
Member Author

Updated 368530d -> 59b55cc (pr18452.02 -> pr18452.03, diff):

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

@promag

promag commented Apr 11, 2020

Copy link
Copy Markdown
Contributor

Tested ACK 59b55cc.

luke-jr
luke-jr reviewed Apr 23, 2020
View reviewed changes
Comment thread src/rpc/server.cpp Outdated
Comment on lines +307 to +311
if (g_rpc_stopped) return;
LogPrint(BCLog::RPC, "Stopping RPC\n");
deadlineTimers.clear();
DeleteAuthCookie();
g_rpc_stopped = true;

@luke-jr luke-jr Apr 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race?

@hebasto hebasto Apr 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@hebasto

hebasto commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

Updated 59b55cc -> 2d7b6bc (pr18452.03 -> pr18452.04, diff):

Race?

This was referenced Apr 29, 2020
Merged
Closed
@hebasto
hebasto force-pushed the 20200327-waitfor branch from 2d7b6bc to da73f15 Compare May 13, 2020 13:15
@hebasto

hebasto commented May 13, 2020

Copy link
Copy Markdown
Member Author

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

@jonasschnelli

jonasschnelli commented May 29, 2020

Copy link
Copy Markdown
Contributor

utACK da73f15

@jonasschnelli
jonasschnelli merged commit e4bfd51 into bitcoin:master May 29, 2020
@hebasto
hebasto deleted the 20200327-waitfor branch May 29, 2020 13:54
@maflcko maflcko removed the GUI label May 29, 2020
@maflcko maflcko changed the title qt: Fix shutdown when waitfor* cmds are called from RPC console rpc: Fix gui shutdown when waitfor* cmds are called from RPC console May 29, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
@hebasto @luke-jr
qt: Fix shutdown when waitfor* cmds are called from RPC console
e5a935c 
Github-Pull: bitcoin#18452
Rebased-From: da73f15
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 24, 2021
@hebasto @PiRK
qt: Fix shutdown when waitfor* cmds are called from RPC console
caf502a 
Summary: This is a backport of [[bitcoin/bitcoin#18452 | core#18452]]

Test Plan:
`bitcoin-qt -server=1`

In the RPC console, type `waitforblockheight XXXXXX`  with XXXXXXX a future block height, then close the application. Make sure it does not hang.

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9260
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@jonasschnelli jonasschnelli jonasschnelli left review comments
@luke-jr luke-jr luke-jr left review comments
@promag promag promag left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

gui: hang after calling any waitfor* cmds from RPC console

6 participants

@hebasto @DrahtBot @jonasschnelli @promag @luke-jr @maflcko