ReusableGoroutinesPool: Fix datarace on Close

[julienduchesne]

Fixes this datarace in Mimir: grafana/mimir#9585, but I figured this went here. Both Go and Close functions are non-blocking, so this shouldn't deadlock

Q: Should this fail open instead? Have new Go calls start new goroutines when the pool is closed. Was that the intented behavior of the struct, or is a panic OK?

Without the mutex:

julienduchesne@triceratops dskit % go test ./concurrency -race
==================
WARNING: DATA RACE
Write at 0x00c00001e240 by goroutine 103:
  runtime.recvDirect()
      /opt/homebrew/opt/go/libexec/src/runtime/chan.go:388 +0x7c
  github.com/grafana/dskit/concurrency.(*ReusableGoroutinesPool).Close()
      /Users/julienduchesne/Repos/dskit/concurrency/worker.go:40 +0x1d4
  github.com/grafana/dskit/concurrency.TestReusableGoroutinesPool_Race()
      /Users/julienduchesne/Repos/dskit/concurrency/worker_test.go:89 +0x1c4
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1743 +0x40

Previous read at 0x00c00001e240 by goroutine 106:
  runtime.chansend1()
      /opt/homebrew/opt/go/libexec/src/runtime/chan.go:157 +0x2c
  github.com/grafana/dskit/concurrency.(*ReusableGoroutinesPool).Go()
      /Users/julienduchesne/Repos/dskit/concurrency/worker.go:29 +0x48
  github.com/grafana/dskit/concurrency.TestReusableGoroutinesPool_Race.func1()
      /Users/julienduchesne/Repos/dskit/concurrency/worker_test.go:82 +0xfc

Goroutine 103 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1743 +0x5e0
  testing.runTests.func1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:2168 +0x80
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1690 +0x184
  testing.runTests()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:2166 +0x6e0
  testing.(*M).Run()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:2034 +0xb74
  go.uber.org/goleak.VerifyTestMain()
      /Users/julienduchesne/go/pkg/mod/go.uber.org/[email protected]/testmain.go:53 +0x44
  github.com/grafana/dskit/concurrency.TestMain()
      /Users/julienduchesne/Repos/dskit/concurrency/limited_concurrency_singleflight_test.go:16 +0x130
  main.main()
      _testmain.go:85 +0x114

Goroutine 106 (running) created at:
  github.com/grafana/dskit/concurrency.TestReusableGoroutinesPool_Race()
      /Users/julienduchesne/Repos/dskit/concurrency/worker_test.go:74 +0x1ac
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1743 +0x40
==================
--- FAIL: TestReusableGoroutinesPool_Race (0.01s)
    testing.go:1399: race detected during execution of test
FAIL
FAIL    github.com/grafana/dskit/concurrency    3.240s
FAIL

[seizethedave]

As Go() is in the hot path of a bunch of services, what do you think about making the mutex an RWLock so that there's not an additional layer of serialization there? Go() can take the reader lock, Close() would take the exclusive.

[julienduchesne]

As Go() is in the hot path of a bunch of services, what do you think about making the mutex an RWLock so that there's not an additional layer of serialization there? Go() can take the reader lock, Close() would take the exclusive.

Done. Also, I've made it spawn new goroutines if the channel is closed rather than panic. It seems like that's what @colega intended with the lib (#606 (comment))

[seizethedave]

LG2M!