eth: fix race condition in eth/sync tests #522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
meowsbits
changed the title
|
I tested this in a super slow machine twice. First time I got a race but test finished. Second time I got a race and test hasn't finished because of timeout after 10 minutes. |
|
Hm, thanks for the report. I'll come back with another. |
The race was discoverable with go test -race ./eth or under 'slow' machine conditions, eg. GOMAXPROCS=1 and/or a 'slower' machine. The race was around the minArtificialFinalityPeers value, which was both - assigned in (during) the tests, and - checked in the nextSyncOp function and the order of those uses was nondeterministic. This patch resolves the issue by moving the adhoc and temporary re-assignment of that package-wide value to the start of the tests, before any go routines for peer handling or sync protocol are fired off. Note that the real-clock timeouts in these tests can still cause spurious test failures under 'slow' conditions (see their 250ms sleeps). Fixes #521 Date: 2023-01-30 13:39:38-08:00 Signed-off-by: meows <b5c6@protonmail.com>
@ziogaschr found a(nother) race condition in the tests, which I have had a hard time dupliating with env GOMAXPROCS=1 go test -count 1 -race ./eth But this should, hopefully, fix the issue. Date: 2023-02-07 08:47:44-08:00 Signed-off-by: meows <b5c6@protonmail.com>
meowsbits
force-pushed
the
fix/sync-test-race-af
branch
from
891b12b
to
ec625f6
Compare
|
Test failed with the following race condition: |
The TestArtificialFinality.._xxx tests are racey because of this value; the tests want to manhandle it while the handler goroutines (and chainsyncop calls) are running. This might fix it, though it feels like overkill. Date: 2023-02-07 13:59:16-08:00 Signed-off-by: meows <b5c6@protonmail.com>
|
I'm still struggling to reproduce the issue, but above is another commit-swing at it. Thanks to @ziogaschr for being my canary in the coal mine. To test, I'm using: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #521
The race was discoverable with
go test -race ./ethor under 'slow' machine conditions, eg. GOMAXPROCS=1 and/or a 'slower' machine.
The race was around the
minArtificialFinalityPeersvalue, which was bothand the order of those uses was nondeterministic.
This patch resolves the issue by moving
the adhoc and temporary re-assignment of that
package-wide value to the start of the tests,
before any go routines for peer handling
or sync protocol are fired off.
Note that the real-clock timeouts in these
tests can still (invariantly) cause spurious test failures
under 'slow' conditions (see their 250ms sleeps).
This invariant limitation (seen in the upstream-identical tests
TestSnapSyncDisabling6[6|7]) can be reproduced on my computer with:Reviewers can test the efficacy of this patch with