Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
May 29, 2020
Merged

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

merged 1 commit into from
May 29, 2020

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 27, 2020 •
edited
Loading

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
Copy link
Member Author

hebasto commented Mar 27, 2020 •
edited
Loading

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 27, 2020 •
edited
Loading

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
Copy link
Member

@promag promag left a comment

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
Copy link
Member Author

hebasto commented Mar 30, 2020

@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
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@jonasschnelli
Copy link
Contributor

Concept ACK

@hebasto
Copy link
Member Author

hebasto commented Apr 8, 2020

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
Copy link
Member

@promag promag left a comment

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.

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
@hebasto
Copy link
Member Author

hebasto commented Apr 10, 2020

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

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

@promag
Copy link
Member

promag commented Apr 11, 2020

Tested ACK 59b55cc.

luke-jr
luke-jr reviewed Apr 23, 2020
View reviewed changes
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;
Copy link
Member

Choose a reason for hiding this comment

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

Race?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@hebasto
Copy link
Member Author

hebasto commented Apr 28, 2020

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

Race?

This was referenced Apr 29, 2020
Merged
Closed
@hebasto
Copy link
Member Author

hebasto commented May 13, 2020

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

@jonasschnelli
Copy link
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 31, 2020
@jonasschnelli
Merge bitcoin#18452: qt: Fix shutdown when waitfor* cmds are called f…
ff0692d 
…rom RPC console

da73f15 qt: Fix shutdown when waitfor* cmds are called from RPC console (Hennadii Stepanov)

Pull request description:

  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 bitcoin#17495

ACKs for top commit:
  jonasschnelli:
    utACK da73f15

Tree-SHA512: 469f5332945a5f2c57d19336cda5df79b123ccc494aea6d58a85eb1293be52708b2b9c5bb6bc2c402a90b7b4e9e8d7ab8fe84cf201cf7ce612c9290c57e43681
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

@jonasschnelli jonasschnelli jonasschnelli left review comments

@luke-jr luke-jr luke-jr left review comments

@promag promag promag left review comments

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