test: Restore unlimited timeout in IndexWaitSynced #28036

1. 
   maflcko commented at 12:20 pm on July 6, 2023: member

   The timeout was unlimited before, so just restore that value for now: #27988 (comment) .

   (Strictly speaking, this is a behavior change for the blockfilterindex and txindex tests, because it only restores the coinstatsindex behavior.)

2. test: Restore unlimited timeout in IndexWaitSynced

   The timeout was unlimited before, so just restore that value for now:
   https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007

   fabed7eb79
3. 
   DrahtBot commented at 12:20 pm on July 6, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy, ajtowns, mzumsande

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #28026 (test: bugfix, synchronize indexes synchronously by furszy)

   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.

4. DrahtBot added the label Tests on Jul 6, 2023
5. 
   furszy commented at 2:05 pm on July 6, 2023: member

   ACK fabed7eb

   I’m a bit more inclined for #28026 but it is fine either way.

   Note: Would be good to mention in the PR description that this is a behavior change for the blockfilterindex and txindex tests. It only restores the coinstatsindex behavior.

6. maflcko requested review from mzumsande on Jul 6, 2023
7. 
   maflcko commented at 2:08 pm on July 6, 2023: member

   Thx, edited OP
8. 
   furszy commented at 2:16 pm on July 6, 2023: member

   Little note: I think that the only downside here with respect to increasing the timeout to a much higher number or #28026 is that the tests could run forever if the index m_synced flag is never set by the thread. I’m not sure how the CI will behave in this scenario, guess that we will realize that something like this happened just by reading the last executed test name.
9. 
   maflcko commented at 2:25 pm on July 6, 2023: member

   We’ve been plagued by intermittent timeouts locally (for months/years) and now in CI (for days), but never a thread that never finished. So I think it should be evident that this or #https://github.com/bitcoin/bitcoin/pull/28026 should be preferred. After all, it is no different than any other deadlock or infinite loop anywhere else in the code.
10. 
    furszy commented at 2:46 pm on July 6, 2023: member

    Actually, it is not when the thread never finishes. The thread could have finished, but never set the m_synced flag to true. Which would make BlockUntilSyncedToCurrentChain never return true. Which could happen on all the index sync errors.
11. 
    ajtowns commented at 2:53 pm on July 6, 2023: contributor

    utACK fabed7eb796637c02e3677ebbe183d90b258ba69

    Could also restore the original behaviour by doing something like:

    0void IndexWaitSynced(BaseIndex& index, std::chrono::microseconds timeout)
    1{
    2    const auto end = SteadyClock::now() + timeout;
    3    while (!index.BlockUntilSyncedToCurrentChain()) {
    4        Assert(timeout <= 0s || SteadyClock::now < end);
    5        UninterruptibleSleep(100ms);
    6    }
    7}
    

    and calling IndexWaitSynced(index, 10s) normally, but IndexWaitSynced(index, -1s) for coinstatsindex.

12. 
    mzumsande commented at 3:07 pm on July 6, 2023: contributor

    ACK fabed7eb796637c02e3677ebbe183d90b258ba69

    as per #28026#pullrequestreview-1514862606 I like this approach more than running the sync synchronously.

    Could also restore the original behaviour by doing something like:

    The problem in the first place was that that the tests for txindex and blockfilterindex would have rare intermittent timeouts. So instead of making guesses about the slowest environment the tests could be run in I think it’s ok to just have no timeout.

13. DrahtBot removed review request from mzumsande on Jul 6, 2023
14. 
    maflcko commented at 3:13 pm on July 6, 2023: member

    Which could happen on all the index sync errors.

    Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?

15. 
    furszy commented at 3:42 pm on July 6, 2023: member

    Which could happen on all the index sync errors.

    Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?

    The quickest fix would be to add a ShutdownRequested() call into the IndexWaitSynced() loop. And verify at the end of the function that the index class is synced by calling:

    0BOOST_CHECK(index.GetSummary().synced);
    

    In this way, we can be guarded against the infinite loop. And also verify the index synchronization result (which must be always successful).

    The “AbortNode/FatalError/StartShutdown” treatment on the tests topic seems to be a different problem that we should aboard in the future, yeah. Here the test main thread will actively-wait for a flag to be set, and the flag could never be set. So, even if we start handling “AbortNode/FatalError/StartShutdown” on the base tests class, the error would need to be handled by a different thread (not sure if we already have something for it).

16. fanquake merged this on Jul 7, 2023
17. fanquake closed this on Jul 7, 2023

    ---

18. sidhujag referenced this in commit 6c97108994 on Jul 7, 2023
19. maflcko deleted the branch on Jul 8, 2023
20. ryanofsky referenced this in commit 99b3af78bd on Jul 11, 2023
21. sidhujag referenced this in commit 7adb772c88 on Jul 12, 2023
22. bitcoin locked this on Jul 7, 2024

Contributors
maflcko DrahtBot furszy ajtowns mzumsande

Labels
Tests