Skip to content

add missing Boost Thread join_all() call during shutdown#3059

Merged
gavinandresen merged 1 commit intobitcoin:masterfrom
Diapolo:Shutdown
Oct 7, 2013
Merged

add missing Boost Thread join_all() call during shutdown#3059
gavinandresen merged 1 commit intobitcoin:masterfrom
Diapolo:Shutdown

Conversation

@Diapolo
Copy link
Copy Markdown

@Diapolo Diapolo commented Oct 6, 2013

@gavinandresen
Copy link
Copy Markdown
Contributor

How did you test the if (!fRet) case (that is, the case of an error during the startup process)?

I tested an alternative version of this patch that just does a join_all in the clean shutdown case by starting up / shutting down 100 times, so ACK on that part.

But I'm not willing to spend the time testing all of the startup-failure cases to make sure they don't result in a hang due to some thread-blocking-waiting-for-another-thread-during-startup case.

@Diapolo
Copy link
Copy Markdown
Author

Diapolo commented Oct 7, 2013

Gavin, I didn't test them, as this patch is based on an observation (missing join_all()) and your comment in my issue-ticket. I'm fine with removing it for if (!fRet) and just placing a comment there instead.

@Diapolo
Copy link
Copy Markdown
Author

Diapolo commented Oct 7, 2013

Updated based on Gavins comments.

@gavinandresen
Copy link
Copy Markdown
Contributor

Nit: it's not true we "can't" test all of the startup failure modes, I actually DID test them all (by simulating all the failure cases with hacked versions of the code) when I did the thread work originally.

Absolutely clean shutdown when there is an error starting up is just too low a priority to justify taking all that time.

- fixes #3037 by adding missing join_all() call and brings bitcoind
  shutdown code in line with Bitcoin-Qt shutdown code
- added a comment for the if (!fRet) case
@Diapolo
Copy link
Copy Markdown
Author

Diapolo commented Oct 7, 2013

Updated comment, to address the nit :).

@BitcoinPullTester
Copy link
Copy Markdown

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c55d1600da35e3882f149b00f972c0ef9cb41dfa for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

gavinandresen added a commit that referenced this pull request Oct 7, 2013
add missing Boost Thread join_all() call during shutdown
@gavinandresen gavinandresen merged commit aa56d31 into bitcoin:master Oct 7, 2013
@Diapolo Diapolo deleted the Shutdown branch October 8, 2013 06:18
Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Apr 8, 2020
Merge master 0.14.0.3 back into develop
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bitcoind shutdown code missing Boost thread join_all()?

3 participants