Data Race When Setting MinIdleConns in redis.Options

[kevinqfu]

Current Behavior

Data race when creating new redistrace client in redis v9 when setting non-0 MinIdleConns

Context (Environment)

Caught this in our unit test using miniredis

Detailed Description

Our code is like this

redistrace.NewClient(&redis.Options{
			Addr:            addr,
			TLSConfig:       tlsConfig,
			PoolSize:        poolSize,
			MinIdleConns:    minIdleConns,
			ConnMaxIdleTime: -1, // Do not close idle connections
			Dialer:          dialer,
		})

When running the code, we got a warning about data race.

WARNING: DATA RACE
Write at 0x00c00035d3f8 by goroutine 102:
  github.com/redis/go-redis/v9.(*hooksMixin).chain()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/redis.go:119 +0x8c
  github.com/redis/go-redis/v9.(*hooksMixin).AddHook()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/redis.go:113 +0x14d
  github.com/redis/go-redis/v9.(*Client).AddHook()
      <autogenerated>:1 +0x2e
  gopkg.in/DataDog/dd-trace-go.v1/contrib/redis/go-redis%2ev9.WrapClient()
      /go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.52.0/contrib/redis/go-redis.v9/redis.go:63 +0x19e
  gopkg.in/DataDog/dd-trace-go.v1/contrib/redis/go-redis%2ev9.NewClient()
      /go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.52.0/contrib/redis/go-redis.v9/redis.go:45 +0x5e

...... <some application code> ......

Previous read at 0x00c00035d3f8 by goroutine 106:
  github.com/redis/go-redis/v9.(*hooksMixin).dialHook()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/redis.go:168 +0x64
  github.com/redis/go-redis/v9.(*hooksMixin).dialHook-fm()
      <autogenerated>:1 +0x99
  github.com/redis/go-redis/v9.newConnPool.func1()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/options.go:503 +0xa4
  github.com/redis/go-redis/v9/internal/pool.(*ConnPool).dialConn()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/internal/pool/pool.go:205 +0x186
  github.com/redis/go-redis/v9/internal/pool.(*ConnPool).addIdleConn()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/internal/pool/pool.go:143 +0x6f
  github.com/redis/go-redis/v9/internal/pool.(*ConnPool).checkMinIdleConns.func1()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/internal/pool/pool.go:126 +0x34

Goroutine 102 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1493 +0x75d
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1846 +0x99
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1844 +0x7ec
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1726 +0xa84
  main.main()
      _testmain.go:109 +0x2e9

Goroutine 106 (running) created at:
  github.com/redis/go-redis/v9/internal/pool.(*ConnPool).checkMinIdleConns()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/internal/pool/pool.go:125 +0x73
  github.com/redis/go-redis/v9/internal/pool.NewConnPool()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/internal/pool/pool.go:109 +0x26e
  github.com/redis/go-redis/v9.newConnPool()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/options.go:501 +0x337
  github.com/redis/go-redis/v9.NewClient()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.2.0/redis.go:623 +0x208
  gopkg.in/DataDog/dd-trace-go.v1/contrib/redis/go-redis%2ev9.NewClient()
      /go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.52.0/contrib/redis/go-redis.v9/redis.go:44 +0x3b

I briefly digged into the code and likely it's more of a problem in the redis library using go routines to create new idle connections https://github.com/redis/go-redis/blob/master/internal/pool/pool.go#L125, then datadog touches this while go routines haven't finished.

[RyoMiyashita]

I encountered the same data race issue with MinIdleConns and client.AddHook. Eager to resolve it, I've developed a fix that I believe addresses the problem effectively. I've added thread-safe measures in hooksMixin.