Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) by LeandroRocha84 · Pull Request #13674 · bitcoin/bitcoin

LeandroRocha84 · 2018-07-16T07:28:37Z

This pull request contains a fix for the issue #13217, where bitcoin-qt becomes unresponsive during shutdown after a long running call (like gettxoutsetinfo) in the QT console.
The problem occurs because, during shutdown, waiting for the RPCExecutor thread to finish happens in the main thread.

LeandroRocha84 · 2018-07-16T17:27:59Z

There's also the possibility of emitting the shutdown signal from RPCExecutor destructor so we don't need a wrapper - that simplify the solution. I created the PR with the wrapper solution because it might be considered unsafe to emit signals from a destructor in a multi-threaded application. Would appreciate any input on this and I'll update the PR accordingly.

promag

Indeed looks like unnecessary complexity. IIUC the main even loop blocks when joining the executor thread, so consider a different approach instead?

QEventLoop loop;
connect(&thread, SIGNAL(finished()), &loop, SLOT(quit()));
if (!thread.isFinished()) loop.exec();

src/qt/rpcconsole.cpp

LeandroRocha84 · 2018-07-17T04:08:07Z

Thanks for the suggestion @promag
I've updated the PR to remove the unnecessary complexity. Aside from your suggested code, I've made a small change to bitcoingui.cpp so that the UI clientModel is not cleared until the RPCExecutor thread joins. Otherwise, network events, like a new connection, that trigger a UI update would raise exceptions.

src/qt/rpcconsole.cpp

src/qt/bitcoingui.cpp

LeandroRocha84 · 2018-09-01T05:45:35Z

Hi all, please let me know if there's anything that needs to be done to have this fix merged.
Thanks

src/qt/rpcconsole.cpp

DrahtBot · 2018-10-24T00:08:43Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15894 (Remove duplicated "Error: " prefix in logs by hebasto)
#9849 (Qt: Network Watch tool by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/qt/bitcoingui.cpp

hebasto · 2018-12-05T08:25:59Z

tACK 18850903f803bba58d110d3994df18969992f383 (on Linux)
Would you squash commits?

LeandroRocha84 · 2018-12-06T07:06:01Z

All comments addressed. Commits squashed. Please let me know if there's anything.

hebasto · 2018-12-06T19:22:18Z

re-tACK abff36969da9998e36c72f2df7faf68564f0c8db (Linux, macOS 10.13.6)

jonasschnelli · 2019-04-16T07:54:11Z

@LeandroRocha84 sorry for keeping this up so long...
can you rebase once and then we try to get this merged?

LeandroRocha84 · 2019-04-17T06:18:39Z

Thanks @jonasschnelli I'm working on solving the conflicts after the rebase.
The order change in fd6d499 breaks the solution in this PR as it makes the QEventLoop.exec returns right away. I've confirmed that the bug this PR intends to solve is still reproducible; the shutdown screen is frozen/blocked when we have a long running RPC command.
I'm going to spend some time this weekend to find out how exactly the order change I linked affects the QEventLoop.

Adding @promag as he might have some insight.

LeandroRocha84 · 2019-04-20T06:18:36Z

Conflicts resolved, rebase completed. I've tested the fix on linux.
@jonasschnelli please let me know if there's anything else I can do to get this pull request completed.
Thank you.

src/qt/bitcoingui.cpp

src/qt/rpcconsole.cpp

src/qt/bitcoingui.cpp

src/qt/bitcoingui.h

src/qt/rpcconsole.cpp

…nning RPC call

DrahtBot · 2019-06-25T11:56:27Z

Needs rebase

DrahtBot · 2019-09-30T13:02:32Z

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

LeandroRocha84 mentioned this pull request Jul 16, 2018

[GUI] QT shutdown is blocked if called before gettxoutsetinfo is finished #13217

Closed

fanquake added the GUI label Jul 16, 2018

promag reviewed Jul 16, 2018

View reviewed changes

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

LeandroRocha84 force-pushed the qt-shutdown-13217 branch 3 times, most recently from 7ebd837 to 3844ba3 Compare July 17, 2018 03:57

fanquake requested a review from jonasschnelli July 17, 2018 06:42

promag reviewed Jul 17, 2018

View reviewed changes

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

LeandroRocha84 force-pushed the qt-shutdown-13217 branch from 3844ba3 to 086d88e Compare July 18, 2018 03:43

DrahtBot mentioned this pull request Jul 22, 2018

Use new Qt5 connect syntax #13529

Merged

promag reviewed Jul 26, 2018

View reviewed changes

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

LeandroRocha84 force-pushed the qt-shutdown-13217 branch from 086d88e to 9255963 Compare July 27, 2018 02:58

hebasto reviewed Sep 22, 2018

View reviewed changes

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Oct 23, 2018

qt: Remove redundant stopThread() and stopExecutor() signals #14250

Merged

DrahtBot mentioned this pull request Oct 31, 2018

Qt: Network Watch tool #9849

Closed

hebasto requested changes Dec 3, 2018

View reviewed changes

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

LeandroRocha84 force-pushed the qt-shutdown-13217 branch from 1885090 to abff369 Compare December 5, 2018 20:31

DrahtBot added the Needs rebase label Jan 17, 2019

LeandroRocha84 force-pushed the qt-shutdown-13217 branch 2 times, most recently from 59ebf17 to a302c99 Compare April 20, 2019 01:03

promag suggested changes Apr 20, 2019

View reviewed changes

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

src/qt/bitcoingui.h Outdated Show resolved Hide resolved

LeandroRocha84 force-pushed the qt-shutdown-13217 branch from a302c99 to ba230e4 Compare April 20, 2019 15:15

DrahtBot removed the Needs rebase label Apr 22, 2019

promag reviewed Apr 24, 2019

View reviewed changes

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

LeandroRocha84 force-pushed the qt-shutdown-13217 branch 4 times, most recently from 937e77a to c42788e Compare May 5, 2019 21:04

Allow GUI to remain responsive during shutdown when there's a long ru…

0636f38

…nning RPC call

LeandroRocha84 force-pushed the qt-shutdown-13217 branch from c42788e to 0636f38 Compare May 5, 2019 21:08

DrahtBot added the Needs rebase label Jun 25, 2019

DrahtBot added the Up for grabs label Sep 30, 2019

DrahtBot closed this Sep 30, 2019

laanwj removed the Needs rebase label Oct 24, 2019

hebasto mentioned this pull request Dec 3, 2019

qt: Do not block GUI thread in RPCConsole #17659

Closed

maflcko removed the Up for grabs label Dec 3, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

LeandroRocha84 commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeandroRocha84 commented Jul 16, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeandroRocha84 commented Jul 17, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeandroRocha84 commented Sep 1, 2018

Uh oh!

Uh oh!

DrahtBot commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

hebasto commented Dec 5, 2018

Uh oh!

LeandroRocha84 commented Dec 6, 2018

Uh oh!

hebasto commented Dec 6, 2018

Uh oh!

jonasschnelli commented Apr 16, 2019

Uh oh!

LeandroRocha84 commented Apr 17, 2019

Uh oh!

LeandroRocha84 commented Apr 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jun 25, 2019

Uh oh!

DrahtBot commented Sep 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

LeandroRocha84 commented Jul 16, 2018 •

edited

Loading

DrahtBot commented Oct 24, 2018 •

edited

Loading