Race condition in AddHook

[duongcongtoai]

I suspect current AddHook function will cause race condition with the background connection pool dialer

WARNING: DATA RACE
Write at 0x00c000150278 by goroutine 84:
  github.com/redis/go-redis/v9.(*hooksMixin).chain()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/redis.go:120 +0x8c
  github.com/redis/go-redis/v9.(*hooksMixin).AddHook()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/redis.go:114 +0x527
  github.com/redis/go-redis/extra/redisotel/v9.InstrumentTracing()
      /go/pkg/mod/github.com/redis/go-redis/extra/redisotel/v9@v9.0.2/tracing.go:28 +0x3e6
...
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47

Previous read at 0x00c000150278 by goroutine 85:
  github.com/redis/go-redis/v9.(*hooksMixin).dialHook()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/redis.go:169 +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.0.2/options.go:489 +0xa4
  github.com/redis/go-redis/v9/internal/pool.(*ConnPool).dialConn()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/internal/pool/pool.go:194 +0x186
  github.com/redis/go-redis/v9/internal/pool.(*ConnPool).addIdleConn()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/internal/pool/pool.go:134 +0x6f
  github.com/redis/go-redis/v9/internal/pool.(*ConnPool).checkMinIdleConns.func1()
      /go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/internal/pool/pool.go:122 +0x34

Possible Solution

Allow options struct to receive a hook on init, instead of letting AddHook function modify the hooks after the pool has been created and running

Steps to Reproduce

Run with race flag:

// Package redisdb is a package for communicating with redis
package redisdb

import (
	"context"
	"time"

	"github.com/redis/go-redis/extra/redisotel/v9"
	"github.com/redis/go-redis/v9"

)

type RedisConfig struct {
	MaxRetriesWrite int
}

// New will create new redis client
func New(ctx context.Context, addr, password string, db int) (*redis.Client, error) {
	rdb := redis.NewClient(&redis.Options{
		Addr:            addr,
		Password:        password,
		DB:              db,
		MaxRetries:      3,
		MinRetryBackoff: time.Second * 3,
		// Maximum backoff between each retry.
		// Default is 512 milliseconds; -1 disables backoff.
		MaxRetryBackoff: time.Second * 3,

		// Dial timeout for establishing new connections.
		// Default is 5 seconds.
		DialTimeout: time.Second * 3,
		// Timeout for socket reads. If reached, commands will fail
		// with a timeout instead of blocking. Supported values:
		//   - `0` - default timeout (3 seconds).
		//   - `-1` - no timeout (block indefinitely).
		//   - `-2` - disables SetReadDeadline calls completely.
		ReadTimeout: time.Second * 3,
		// Timeout for socket writes. If reached, commands will fail
		// with a timeout instead of blocking.  Supported values:
		//   - `0` - default timeout (3 seconds).
		//   - `-1` - no timeout (block indefinitely).
		//   - `-2` - disables SetWriteDeadline calls completely.
		WriteTimeout: time.Second * 3,

		ContextTimeoutEnabled: true,

		// Maximum number of socket connections.
		// Default is 10 connections per every available CPU as reported by runtime.GOMAXPROCS.
		PoolSize: 10,
		// Minimum number of idle connections which is useful when establishing
		// new connection is slow.
		MinIdleConns: 1,
		// Maximum number of idle connections.
		MaxIdleConns: 1,
		// ConnMaxIdleTime is the maximum amount of time a connection may be idle.
		// Should be less than server's timeout.
		//
		// Expired connections may be closed lazily before reuse.
		// If d <= 0, connections are not closed due to a connection's idle time.
		//
		// Default is 30 minutes. -1 disables idle timeout check.
		ConnMaxIdleTime: time.Second * 30,
		// ConnMaxLifetime is the maximum amount of time a connection may be reused.
		//
		// Expired connections may be closed lazily before reuse.
		// If <= 0, connections are not closed due to a connection's age.
		//
		// Default is to not close idle connections.
		ConnMaxLifetime: time.Minute * 15,
	})

	// Check if redis not avail fail the program
	if _, err := rdb.Ping(ctx).Result(); err != nil {
		return nil, err
	}
	// Enable tracing instrumentation.
	if err := redisotel.InstrumentTracing(rdb); err != nil {
		return nil, err
	}

	return rdb, nil
}

Context (Environment)

go 1.19
	github.com/redis/go-redis/extra/redisotel/v9 v9.0.2
	github.com/redis/go-redis/v9 v9.0.2

[monkey92t]

Yes, go-redis.Hook is not safe for multiple goroutines concurrently. We should execute the redis command after adding all hooks.

[duongcongtoai]

I think the dialing function will be called regardless the redis command is called or not, it happens in background (and asap during startup)

func (p *ConnPool) checkMinIdleConns() {
	if p.cfg.MinIdleConns == 0 {
		return
	}
	for p.poolSize < p.cfg.PoolSize && p.idleConnsLen < p.cfg.MinIdleConns {
		p.poolSize++
		p.idleConnsLen++

		go func() {
			err := p.addIdleConn()

[monkey92t]

I think the dialing function will be called regardless the redis command is called or not, it happens in background (and asap during startup)

func (p *ConnPool) checkMinIdleConns() {
	if p.cfg.MinIdleConns == 0 {
		return
	}
	for p.poolSize < p.cfg.PoolSize && p.idleConnsLen < p.cfg.MinIdleConns {
		p.poolSize++
		p.idleConnsLen++

		go func() {
			err := p.addIdleConn()

Oh... It's MinIdleConns again, MinIdleConns caused a lot of accidents😂
#2447 Also caused by MinIdleConns..

[monkey92t]

@vmihailenco MinIdleConns and DialHook are conflicting, we can remove checkMinIdleConns check in NewConnPool.

Why do we need DialHook?

[vmihailenco]

Why do we need DialHook?

It is a hook that wraps the Dial method. go-redis does not use that hook, but that's true for all other hooks.

We probably need to looks into making AddHook thread-safe.

[HenriBeck]

@vmihailenco one option would also be to expose a way to add hooks before checkMinIdleConns is called, e.g. as an option on the redis.Options.

While this won't work for every case, it might be a good step to solve some of the issues.

[duongcongtoai]

fixed on #2814