Page MenuHomePhabricator

test: Fix intermittent wallet_multiwallet issue with got_loading_error
ClosedPublic

Authored by PiRK on Dec 15 2021, 14:42.

Details

Summary

Failing the test after 10 iterations without a loading error is problematic because it may take 11 iterations to get a loading error.

Fix that by running until a loading error occurs, which should happen in almost all runs within the first 10 iterations.

This is a backport of core#20569

Test Plan

ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Dec 15 2021, 14:42

Unless I'm missing something, there is no timeout ? So if the race is fixed this will be an infinite loop ?

PiRK edited the summary of this revision. (Show Details)

add a timeout and a rationale for the timeout in the summary.

Fabien requested changes to this revision.Dec 17 2021, 09:32

If you're running to the timeout you will exhaust the CPU, you need a sleep in the loop

This revision now requires changes to proceed.Dec 17 2021, 09:32

If you're running to the timeout you will exhaust the CPU, you need a sleep in the loop

Will that not significantly lower the probability to trigger a race between the threads?

PiRK edited the summary of this revision. (Show Details)

Add a small sleep in the loop to avoid CPU exhaustion in case the race never happens. I tried to keep the time as small as possible so as to not reduce the likelihood of the race. As far as I understand, 1 ms is below the minimum sleep threshold that most OS can respect, so there is no point of making it smaller than that.

I did some benchmarking and found that a typcal load/unload cycle takes about 10 ms. While testing, I found that the race always happened during the very first cycle, so the chances of hitting that sleep are small.

I also simplified the loop's exit condition, by replacing while True: if exit_condition: return with while not exit_condition:

This revision is now accepted and ready to land.Dec 20 2021, 10:25