search

goburrow / cache

Mango Cache 🥭 - Partial implementation of Guava Cache in Go (golang).
BSD 3-Clause "New" or "Revised" License
574 stars 48 forks source link

Data race in cache.getEntry() and cache.setEntry() #21

Closed aka-bo closed 4 years ago

aka-bo commented 4 years ago

Running the existing benchmarks with the -race flag will expose the data race.

$ go test -race -bench=.
==================
==================
WARNING: DATA RACE
Write at 0x00c00017b8a8 by goroutine 29:
  github.com/goburrow/cache.setEntry()
      /Users/aka-bo/github.com/goburrow/cache/policy.go:31 +0x8c7
  github.com/goburrow/cache.(*slruCache).hit()
      /Users/aka-bo/github.com/goburrow/cache/lru.go:182 +0x8b6
  github.com/goburrow/cache.(*localCache).hit()
      /Users/aka-bo/github.com/goburrow/cache/local.go:247 +0x11b
  github.com/goburrow/cache.(*localCache).processEntries()
      /Users/aka-bo/github.com/goburrow/cache/local.go:188 +0x31f

Previous read at 0x00c00017b8a8 by goroutine 30:
  github.com/goburrow/cache.getEntry()
      /Users/aka-bo/github.com/goburrow/cache/policy.go:26 +0x111
  github.com/goburrow/cache.(*localCache).GetIfPresent()
      /Users/aka-bo/github.com/goburrow/cache/local.go:104 +0x104
  github.com/goburrow/cache.benchmarkCache.func2()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:70 +0xca
  testing.(*B).RunParallel.func1()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:763 +0x182

Goroutine 29 (running) created at:
  github.com/goburrow/cache.(*localCache).init()
      /Users/aka-bo/github.com/goburrow/cache/local.go:77 +0x353
  github.com/goburrow/cache.New()
      /Users/aka-bo/github.com/goburrow/cache/local.go:349 +0x1b0
  github.com/goburrow/cache.benchmarkCache()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:50 +0x8b
  github.com/goburrow/cache.BenchmarkUniformLess()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:24 +0x62
  testing.(*B).runN()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:191 +0x1b4
  testing.(*B).launch()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:321 +0x15c

Goroutine 30 (running) created at:
  testing.(*B).RunParallel()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:756 +0x256
  github.com/goburrow/cache.benchmarkCache()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:67 +0x298
  github.com/goburrow/cache.BenchmarkUniformLess()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:24 +0x62
  testing.(*B).runN()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:191 +0x1b4
  testing.(*B).launch()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:321 +0x15c
==================
--- FAIL: BenchmarkUniformLess-12
    benchmark_test.go:84: total: 10000 (53.130916ms), hits: 9477 (94.77%), misses: 523 (5.23%), evictions: 0
    benchmark.go:197: race detected during execution of benchmark
BenchmarkCounter-12                   125546          8638 ns/op         122 B/op          4 allocs/op
--- BENCH: BenchmarkCounter-12
    benchmark_test.go:84: total: 10000 (95.504204ms), hits: 0 (0.00%), misses: 10000 (100.00%), evictions: 9488
    benchmark_test.go:84: total: 125546 (1.084401292s), hits: 0 (0.00%), misses: 125546 (100.00%), evictions: 125034
BenchmarkExponential-12               432282          2855 ns/op           2 B/op          0 allocs/op
--- BENCH: BenchmarkExponential-12
    benchmark_test.go:84: total: 10000 (27.719805ms), hits: 9987 (99.87%), misses: 13 (0.13%), evictions: 0
    benchmark_test.go:84: total: 432282 (1.234047091s), hits: 432267 (100.00%), misses: 15 (0.00%), evictions: 0
BenchmarkZipf-12                      294660          3836 ns/op          38 B/op          1 allocs/op
--- BENCH: BenchmarkZipf-12
    benchmark_test.go:84: total: 10000 (40.668371ms), hits: 6802 (68.02%), misses: 3198 (31.98%), evictions: 2679
    benchmark_test.go:84: total: 294660 (1.130175798s), hits: 213033 (72.30%), misses: 81627 (27.70%), evictions: 81046
BenchmarkHotspot-12                 ==================
WARNING: DATA RACE
Write at 0x00c000098e88 by goroutine 38:
  github.com/goburrow/cache.setEntry()
      /Users/aka-bo/github.com/goburrow/cache/policy.go:31 +0x85a
  github.com/goburrow/cache.(*slruCache).accessNoLock()
      /Users/aka-bo/github.com/goburrow/cache/lru.go:222 +0x849
  github.com/goburrow/cache.(*slruCache).add()
      /Users/aka-bo/github.com/goburrow/cache/lru.go:125 +0x243
  github.com/goburrow/cache.(*localCache).add()
      /Users/aka-bo/github.com/goburrow/cache/local.go:205 +0x17d
  github.com/goburrow/cache.(*localCache).processEntries()
      /Users/aka-bo/github.com/goburrow/cache/local.go:185 +0x353

Previous read at 0x00c000098e88 by goroutine 79:
  github.com/goburrow/cache.getEntry()
      /Users/aka-bo/github.com/goburrow/cache/policy.go:26 +0x111
  github.com/goburrow/cache.(*localCache).GetIfPresent()
      /Users/aka-bo/github.com/goburrow/cache/local.go:104 +0x104
  github.com/goburrow/cache.benchmarkCache.func2()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:70 +0xca
  testing.(*B).RunParallel.func1()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:763 +0x182

Goroutine 38 (running) created at:
  github.com/goburrow/cache.(*localCache).init()
      /Users/aka-bo/github.com/goburrow/cache/local.go:77 +0x353
  github.com/goburrow/cache.New()
      /Users/aka-bo/github.com/goburrow/cache/local.go:349 +0x1b0
  github.com/goburrow/cache.benchmarkCache()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:50 +0x8b
  github.com/goburrow/cache.BenchmarkHotspot()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:46 +0x70
  testing.(*B).runN()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:191 +0x1b4
  testing.(*B).launch()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:321 +0x15c

Goroutine 79 (running) created at:
  testing.(*B).RunParallel()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:756 +0x256
  github.com/goburrow/cache.benchmarkCache()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:67 +0x298
  github.com/goburrow/cache.BenchmarkHotspot()
      /Users/aka-bo/github.com/goburrow/cache/benchmark_test.go:46 +0x70
  testing.(*B).runN()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:191 +0x1b4
  testing.(*B).launch()
      /usr/local/Cellar/go/1.14.2_1/libexec/src/testing/benchmark.go:321 +0x15c
==================
--- FAIL: BenchmarkHotspot-12
    benchmark_test.go:84: total: 10000 (44.992688ms), hits: 8009 (80.09%), misses: 1991 (19.91%), evictions: 1470
    benchmark_test.go:84: total: 269086 (981.288489ms), hits: 223828 (83.18%), misses: 45258 (16.82%), evictions: 44674
    benchmark_test.go:84: total: 329036 (1.156944467s), hits: 273982 (83.27%), misses: 55054 (16.73%), evictions: 54447
    benchmark.go:197: race detected during execution of benchmark
BenchmarkSumInt-12                  100000000           13.9 ns/op         0 B/op          0 allocs/op
BenchmarkSumString-12               59374192            20.7 ns/op         0 B/op          0 allocs/op
BenchmarkCountMinSketchReset-12        15427         72324 ns/op           0 B/op          0 allocs/op
FAIL
exit status 1
FAIL    github.com/goburrow/cache   10.919s
nqv commented 4 years ago

Thank you. I believe this is fixed in https://github.com/goburrow/cache/commit/ed586e32006b30416a68137394a5f27d374c6eb0