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.

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

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

Merged
merged 2 commits into from Jan 17, 2019

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Sep 17, 2018

The QThread::finished signal do this work.

@fanquake fanquake added the GUI label Sep 17, 2018
src/qt/bitcoin.cpp Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 18, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13674 (Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) by LeandroRocha84)

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.

@fanquake fanquake requested a review from jonasschnelli Sep 18, 2018
@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 20, 2018

seems more elegant, utACK 041be348b0bd96d87eb3d1a2b0edc47538dc9f66 will test soon.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 25, 2018

Tested ACK #041be348b0bd96d87eb3d1a2b0edc47538dc9f66
https://bitcoin.jonasschnelli.ch/build/795

@jonasschnelli jonasschnelli removed their request for review Sep 25, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 28, 2018

Coverage Change (pull 14250) Reference (master)
Lines +0.0044 % 87.0361 %
Functions +0.0154 % 84.1130 %
Branches -0.0038 % 51.5451 %
@hebasto
Copy link
Member Author

@hebasto hebasto commented Oct 12, 2018

@Sjors Could you possibly to review this PR?

@Sjors
Copy link
Member

@Sjors Sjors commented Oct 13, 2018

Definitely looks cleaner, but I'm not sure what to test: quitting the app still works on macOS 10.14 in 041be348b0bd96d87eb3d1a2b0edc47538dc9f66, so that's good.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Oct 17, 2018

Needs more tests (@Sjors, @fanquake, etc.)...

@fanquake
Copy link
Member

@fanquake fanquake commented Oct 18, 2018

tACK 041be34

Tested on top of 816fab9. If anyone has extra tests etc that could be run against this please post them here.

@fanquake fanquake requested a review from laanwj Oct 18, 2018
@fanquake fanquake added this to Mergeable in High-priority for review Oct 19, 2018
@hebasto hebasto changed the title qt: Remove redundant stopThread() signal qt: Remove redundant stopThread() and stopExecutor() signals Oct 21, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Oct 21, 2018

Added similar cleanup for RPCExecutor. Please re-review.

Copy link
Contributor

@ken2812221 ken2812221 left a comment

tACK 4512ac8bdec9446674e774a83164419caae4b4ed

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Dec 3, 2018

Concept ACK

@hebasto hebasto force-pushed the hebasto:stopthread-signal branch Dec 7, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Dec 7, 2018

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Dec 7, 2018
@hebasto hebasto force-pushed the hebasto:stopthread-signal branch Jan 6, 2019
@hebasto
Copy link
Member Author

@hebasto hebasto commented Jan 6, 2019

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Jan 6, 2019
@MarcoFalke MarcoFalke closed this Jan 6, 2019
@MarcoFalke MarcoFalke reopened this Jan 6, 2019
hebasto added 2 commits Sep 17, 2018
@hebasto hebasto force-pushed the hebasto:stopthread-signal branch to 24313fb Jan 9, 2019
@hebasto
Copy link
Member Author

@hebasto hebasto commented Jan 9, 2019

Rebased. Again :)

@DrahtBot DrahtBot removed the Needs rebase label Jan 9, 2019
@laanwj
Copy link
Member

@laanwj laanwj commented Jan 17, 2019

utACK 24313fb

@laanwj laanwj merged commit 24313fb into bitcoin:master Jan 17, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jan 17, 2019
…gnals

24313fb Remove redundant stopExecutor() signal (Hennadii Stepanov)
1c0e0a5 Remove redundant stopThread() signal (Hennadii Stepanov)

Pull request description:

  The `QThread::finished` signal do this work.

Tree-SHA512: 1afce23d30232276d50c3af5af79d83b88e390a2b71f7df585cc1079585d330447d179bbc34c0a89599beb2da035dfd5b9ce23238171490825cabc3a19ae6e67
@hebasto hebasto deleted the hebasto:stopthread-signal branch Jan 17, 2019
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 9, 2020
Summary:
PR description:
> The QThread::finished signal do this work.

Backport of Core [[bitcoin/bitcoin#14250 | PR14250]], part 1 of 2
Commit [[bitcoin/bitcoin@1c0e0a5 | 1c0e0a5]]

Test Plan:
`ninja && ninja check`

Then run `src/qt/bitcoin-qt` and make sure there are no errors on shutdown.

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7844
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 9, 2020
Summary:
PR description:
> The QThread::finished signal do this work.

This concludes backport of Core [[bitcoin/bitcoin#14250 | PR14250]]

Commit [[bitcoin/bitcoin@24313fb | 24313fb]]

Depends on D7844

Test Plan:
`ninja && ninja check`

Run `src/qt/bitcoin-qt -regtest`, open the console and run some RPC commands,
make sure the shutdown works without issues.

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7845
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.