- 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
add missing Boost Thread join_all() call during shutdown #3059
pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:Shutdown changing 1 files +13 −4-
Diapolo commented at 12:20 PM on October 6, 2013: none
-
gavinandresen commented at 2:50 AM on October 7, 2013: 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 commented at 6:08 AM on October 7, 2013: none
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 forif (!fRet)and just placing a comment there instead. -
Diapolo commented at 6:13 AM on October 7, 2013: none
Updated based on Gavins comments.
-
gavinandresen commented at 6:39 AM on October 7, 2013: 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.
-
c55d1600da
add missing Boost Thread join_all() call during shutdown
- 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 commented at 6:53 AM on October 7, 2013: none
Updated comment, to address the nit :).
-
BitcoinPullTester commented at 12:54 PM on October 7, 2013: none
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 referenced this in commit aa56d317a5 on Oct 7, 2013
- gavinandresen merged this on Oct 7, 2013
- gavinandresen closed this on Oct 7, 2013
- Diapolo deleted the branch on Oct 8, 2013
- Bushstar referenced this in commit 4588f67f84 on Apr 8, 2020
- DrahtBot locked this on Sep 8, 2021
