Fix stream race tests #1208
Fix stream race tests #1208
Conversation
swarm/storage/ldbstore.go
Outdated
| @@ -1049,7 +1049,9 @@ func (s *LDBStore) Close() { | |||
| s.lock.Unlock() | |||
| // force writing out current batch | |||
| s.writeCurrentBatch() | |||
| s.lock.Lock() | |||
There was a problem hiding this comment.
It is strange, but this is the data race: #1198 (comment).
| @@ -92,6 +93,15 @@ func TestSyncingViaGlobalSync(t *testing.T) { | |||
| if *longrunning { | |||
| chnkCnt = []int{1, 8, 32, 256, 1024} | |||
| nodeCnt = []int{16, 32, 64, 128, 256} | |||
| } else if raceTest { | |||
There was a problem hiding this comment.
is this still needed on top of using db vs in memory store?
swarm/network/stream/syncer_test.go
Outdated
| testSyncBetweenNodes(t, 16, dataChunkCount, true, 1) | ||
| // This test uses much more memory when running with | ||
| // race detector. Allow it to finish successfully by | ||
| // reducing it s scope, and still check for data races |
There was a problem hiding this comment.
LGTM
Respect for all the work went into this PR! You increase the code/test stability by magnitude.
I like the way you figured how to differentiate between -race and no race mode.
If I go nitpicking as you kinda lie down a standard there - we were able to avoid this differentiation before - I have a minor comment regarding filenames.
By looking at /usr/lib/go/src/internal/race/norace.go and /usr/lib/go/src/internal/race/race.go I would pick the following names for us:
race_test.gonorace_test.go- I would also prefer
const raceEnabledbut that I can easily let go.
|
Thank you @frncmx for a detailed review. I like your suggestions about naming and using constants. I had a trouble with finding the right names, but I did not look into stdlib. |
|
drop closing of |
|
@zelig thanks, |
|
Submitted upstream ethereum/go-ethereum#19051. |
This PR should solve races and deadlock in the stream package. More details on specific problems in #1198. Every commit in this PR is addressing a different issue, but only the whole PR produces a successful
streamtest runs with race detectors.Tests TestSyncingViaGlobalSync and TestSyncerSimulation use a a lot of memory when they are run with race detector, but the largest memory consumer is the leveldb, so this tests have different configuration when running in race mode.