Coming from #28036 (comment), I thought that we were going to fix it there but seems that got merged without it for some reason.
As index sync failures trigger a shutdown request without notifying BaseIndex::BlockUntilSyncedToCurrentChain in any way, we also need to check whether a shutdown was requested or not inside ‘IndexWaitSynced’.
Otherwise, any error inside the index sync process will hang the test forever.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | jamesob, MarcoFalke, ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
12 {
13- while (!index.BlockUntilSyncedToCurrentChain()) {
14+ // As index sync failures trigger a shutdown request without notifying BlockUntilSyncedToCurrentChain in any way,
15+ // we also need to check whether a shutdown was requested or not. Otherwise, any error inside the index sync process
16+ // will hang the test forever.
17+ while (!ShutdownRequested() && !index.BlockUntilSyncedToCurrentChain()) {
Assert(!ShutdownRequested()) inside the loop, to give a better error message when an internal error causes a shutdown, instead of breaking out of the loop and then asserting synced?
Nice, thank you.
lgtm ACK 4b405318d4c476351cccc30588f9126edd94cc35
Can be tested on current master with:
0diff --git a/src/index/base.cpp b/src/index/base.cpp
1index cf07cae286..2987eeb97b 100644
2--- a/src/index/base.cpp
3+++ b/src/index/base.cpp
4@@ -234,6 +234,7 @@ void BaseIndex::ThreadSync()
5 __func__, pindex->GetBlockHash().ToString());
6 return;
7 }
8+ return FatalErrorf("foobar");
9 }
10 }
11
Result on master:
0$ ./src/test/test_bitcoin -t blockfilter_index_tests
1Running 2 test cases...
2Error: A fatal internal error occurred, see debug.log for details
3
4
5...
6
7
8Never finishes ...
9...
10^C
Result with this pull applied, immediate crash:
0$ ./src/test/test_bitcoin -t blockfilter_index_tests
1Running 2 test cases...
2Error: A fatal internal error occurred, see debug.log for details
3test/util/index.cpp:18 IndexWaitSynced: Assertion `!ShutdownRequested()' failed.
4unknown location(0): fatal error: in "blockfilter_index_tests/blockfilter_index_initial_sync": signal: SIGABRT (application abort requested)
5test/blockfilter_index_tests.cpp(142): last checkpoint
6test_bitcoin: common/args.cpp:577: void ArgsManager::AddArg(const std::string &, const std::string &, unsigned int, const OptionsCategory &): Assertion `ret.second' failed.
7unknown location(0): fatal error: in "blockfilter_index_tests/blockfilter_index_init_destroy": signal: SIGABRT (application abort requested)
8test/blockfilter_index_tests.cpp(272): last checkpoint: "blockfilter_index_init_destroy" fixture ctor
9
10*** 2 failures are detected in the test module "Bitcoin Core Test Suite"
11test_bitcoin: checkqueue.h:198: CCheckQueue<CScriptCheck>::~CCheckQueue() [T = CScriptCheck]: Assertion `m_worker_threads.empty()' failed.
12Aborted (core dumped)
10 #include <util/time.h>
11
12 void IndexWaitSynced(const BaseIndex& index)
13 {
14 while (!index.BlockUntilSyncedToCurrentChain()) {
15+ // As index sync failures trigger a shutdown request without notifying BlockUntilSyncedToCurrentChain in any way,
In commit “test: indexes, fix on error infinite loop” (4b405318d4c476351cccc30588f9126edd94cc35)
I found “As index sync failures” comment difficult to understand because it was hard to see how it directly related to code. Would suggest something more literal like “Assert shutdown was not requested to abort the test, instead of looping forever, in case there was an unexpected error in the index that caused it to stop syncing and request a shutdown”
As index sync failures trigger a shutdown request without notifying
BaseIndex::BlockUntilSyncedToCurrentChain in any way, we also need
to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test
forever.
ACK 89ba890
Built/tested locally. Seems like a commonsense way to assert-fail instead of causing an infinite loop on CI machines.