feat: cache: redis: configurable client timeouts and idle connection recycling

clients/redis.go

@@ -3,24 +3,25 @@ package clients
 import (
 	"context"
 	"fmt"
+	"time"
 
 	"github.com/contentsquare/chproxy/config"
 	"github.com/redis/go-redis/v9"
 )
 
-func NewRedisClient(cfg config.RedisCacheConfig) (redis.UniversalClient, error) {
-	options := &redis.UniversalOptions{
-		Addrs:      cfg.Addresses,
-		Username:   cfg.Username,
-		Password:   cfg.Password,
-		PoolSize:   cfg.PoolSize,
-		MaxRetries: 7, // default value = 3, since MinRetryBackoff = 8 msec & MinRetryBackoff = 512 msec
-		// the redis client will wait up to 1016 msec btw the 7 tries
-	}
+const (
+	// default value = 3, since MinRetryBackoff = 8 msec & MinRetryBackoff = 512 msec
+	// the redis client will wait up to 1016 msec btw the 7 tries
+	defaultRedisMaxRetries = 7
 
-	if len(cfg.Addresses) == 1 {
-		options.DB = cfg.DBIndex
-	}
+	// go-redis keeps idle connections for 30 minutes by default, which is longer than
+	// the idle timeout of many firewalls/NATs (e.g. 10 minutes on GCP). Silently dropped
+	// connections then stall commands until they time out, so recycle them earlier.
+	defaultRedisConnMaxIdleTime = 5 * time.Minute
+)
+
+func NewRedisClient(cfg config.RedisCacheConfig) (redis.UniversalClient, error) {
+	options := redisOptions(cfg)
 
 	// maintain backwards compatibility in case of non-presence of enable_tls
 	if len(cfg.CertFile) != 0 || len(cfg.KeyFile) != 0 || cfg.EnableTLS {
@@ -41,3 +42,32 @@ func NewRedisClient(cfg config.RedisCacheConfig) (redis.UniversalClient, error)
 
 	return r, nil
 }
+
+func redisOptions(cfg config.RedisCacheConfig) *redis.UniversalOptions {
+	options := &redis.UniversalOptions{
+		Addrs:           cfg.Addresses,
+		Username:        cfg.Username,
+		Password:        cfg.Password,
+		PoolSize:        cfg.PoolSize,
+		MaxRetries:      cfg.MaxRetries,
+		DialTimeout:     time.Duration(cfg.DialTimeout),
+		ReadTimeout:     time.Duration(cfg.ReadTimeout),
+		WriteTimeout:    time.Duration(cfg.WriteTimeout),
+		PoolTimeout:     time.Duration(cfg.PoolTimeout),
+		ConnMaxIdleTime: time.Duration(cfg.ConnMaxIdleTime),
+	}
+
+	if options.MaxRetries == 0 {
+		options.MaxRetries = defaultRedisMaxRetries
+	}
+
+	if options.ConnMaxIdleTime == 0 {
+		options.ConnMaxIdleTime = defaultRedisConnMaxIdleTime
+	}
+
+	if len(cfg.Addresses) == 1 {
+		options.DB = cfg.DBIndex
+	}
+
+	return options
+}

clients/redis_test.go

@@ -0,0 +1,69 @@
+package clients
+
+import (
+	"testing"
+	"time"
+
+	"github.com/contentsquare/chproxy/config"
+)
+
+func TestRedisOptionsDefaults(t *testing.T) {
+	opts := redisOptions(config.RedisCacheConfig{Addresses: []string{"127.0.0.1:6379"}})
+
+	if opts.MaxRetries != defaultRedisMaxRetries {
+		t.Errorf("expected MaxRetries %d, got %d", defaultRedisMaxRetries, opts.MaxRetries)
+	}
+	if opts.ConnMaxIdleTime != defaultRedisConnMaxIdleTime {
+		t.Errorf("expected ConnMaxIdleTime %s, got %s", defaultRedisConnMaxIdleTime, opts.ConnMaxIdleTime)
+	}
+	// zero values let go-redis apply its own defaults
+	if opts.DialTimeout != 0 || opts.ReadTimeout != 0 || opts.WriteTimeout != 0 || opts.PoolTimeout != 0 {
+		t.Errorf("expected zero timeouts, got dial=%s read=%s write=%s pool=%s",
+			opts.DialTimeout, opts.ReadTimeout, opts.WriteTimeout, opts.PoolTimeout)
+	}
+}
+
+func TestRedisOptionsOverrides(t *testing.T) {
+	cfg := config.RedisCacheConfig{
+		Addresses:       []string{"127.0.0.1:6379"},
+		DialTimeout:     config.Duration(2 * time.Second),
+		ReadTimeout:     config.Duration(time.Second),
+		WriteTimeout:    config.Duration(1500 * time.Millisecond),
+		PoolTimeout:     config.Duration(3 * time.Second),
+		ConnMaxIdleTime: config.Duration(4 * time.Minute),
+		MaxRetries:      -1,
+	}
+
+	opts := redisOptions(cfg)
+
+	if opts.DialTimeout != 2*time.Second {
+		t.Errorf("expected DialTimeout 2s, got %s", opts.DialTimeout)
+	}
+	if opts.ReadTimeout != time.Second {
+		t.Errorf("expected ReadTimeout 1s, got %s", opts.ReadTimeout)
+	}
+	if opts.WriteTimeout != 1500*time.Millisecond {
+		t.Errorf("expected WriteTimeout 1.5s, got %s", opts.WriteTimeout)
+	}
+	if opts.PoolTimeout != 3*time.Second {
+		t.Errorf("expected PoolTimeout 3s, got %s", opts.PoolTimeout)
+	}
+	if opts.ConnMaxIdleTime != 4*time.Minute {
+		t.Errorf("expected ConnMaxIdleTime 4m, got %s", opts.ConnMaxIdleTime)
+	}
+	if opts.MaxRetries != -1 {
+		t.Errorf("expected MaxRetries -1, got %d", opts.MaxRetries)
+	}
+}
+
+func TestRedisOptionsDBIndexOnlyForSingleAddress(t *testing.T) {
+	single := redisOptions(config.RedisCacheConfig{Addresses: []string{"a:6379"}, DBIndex: 3})
+	if single.DB != 3 {
+		t.Errorf("expected DB 3 for a single address, got %d", single.DB)
+	}
+
+	cluster := redisOptions(config.RedisCacheConfig{Addresses: []string{"a:6379", "b:6379"}, DBIndex: 3})
+	if cluster.DB != 0 {
+		t.Errorf("expected DB 0 for multiple addresses, got %d", cluster.DB)
+	}
+}

config/README.md

@@ -122,6 +122,17 @@ redis:
   password: <string>
   pool_size: <int>
   db_index: <int> | default = 0 [optional] # This option is only applicable for non-clustered Redis instance.
+  # Timeouts of the redis client. When omitted, the go-redis defaults apply.
+  dial_timeout: <duration> | default = 5s [optional]
+  read_timeout: <duration> | default = 3s [optional]
+  write_timeout: <duration> | default = read_timeout [optional]
+  pool_timeout: <duration> | default = read_timeout + 1s [optional] # max time to wait for a free connection from the pool
+  # Max time a pooled connection may stay idle before being closed.
+  # Keep it below the idle timeout of any firewall/NAT between chproxy and redis
+  # (e.g. 10m on GCP), otherwise commands may stall on silently dropped connections.
+  conn_max_idle_time: <duration> | default = 5m [optional]
+  # Max number of retries of a failed command. -1 disables retries.
+  max_retries: <int> | default = 7 [optional]
 
 # Expiration time for cached responses.
 expire: <duration>

config/config.go

@@ -966,12 +966,25 @@ type RedisCacheConfig struct {
 	TLS       `yaml:",inline"`
 	EnableTLS bool `yaml:"enable_tls,omitempty"`
 
-	Username  string                 `yaml:"username,omitempty"`
-	Password  string                 `yaml:"password,omitempty"`
-	Addresses []string               `yaml:"addresses"`
-	DBIndex   int                    `yaml:"db_index,omitempty"`
-	PoolSize  int                    `yaml:"pool_size,omitempty"`
-	XXX       map[string]interface{} `yaml:",inline"`
+	Username  string   `yaml:"username,omitempty"`
+	Password  string   `yaml:"password,omitempty"`
+	Addresses []string `yaml:"addresses"`
+	DBIndex   int      `yaml:"db_index,omitempty"`
+	PoolSize  int      `yaml:"pool_size,omitempty"`
+
+	// Timeouts of the redis client. Zero values fall back to the go-redis defaults.
+	DialTimeout  Duration `yaml:"dial_timeout,omitempty"`
+	ReadTimeout  Duration `yaml:"read_timeout,omitempty"`
+	WriteTimeout Duration `yaml:"write_timeout,omitempty"`
+	PoolTimeout  Duration `yaml:"pool_timeout,omitempty"`
+
+	// ConnMaxIdleTime is the max time a pooled connection may stay idle before being closed.
+	ConnMaxIdleTime Duration `yaml:"conn_max_idle_time,omitempty"`
+
+	// MaxRetries is the max number of retries of a failed command. -1 disables retries.
+	MaxRetries int `yaml:"max_retries,omitempty"`
+
+	XXX map[string]interface{} `yaml:",inline"`
 }
 
 // UnmarshalYAML implements the yaml.Unmarshaler interface.

config/config_test.go

@@ -50,10 +50,16 @@ var fullConfig = Config{
 			MaxPayloadSize:     ByteSize(100 << 30),
 			SharedWithAllUsers: true,
 			Redis: RedisCacheConfig{
-				Username:  "chproxy",
-				Password:  "password",
-				Addresses: []string{"127.0.0.1:" + redisPort},
-				PoolSize:  10,
+				Username:        "chproxy",
+				Password:        "password",
+				Addresses:       []string{"127.0.0.1:" + redisPort},
+				PoolSize:        10,
+				DialTimeout:     Duration(2 * time.Second),
+				ReadTimeout:     Duration(time.Second),
+				WriteTimeout:    Duration(time.Second),
+				PoolTimeout:     Duration(2 * time.Second),
+				ConnMaxIdleTime: Duration(4 * time.Minute),
+				MaxRetries:      2,
 			},
 		},
 	},
@@ -926,6 +932,12 @@ caches:
     addresses:
     - 127.0.0.1:%s
     pool_size: 10
+    dial_timeout: 2s
+    read_timeout: 1s
+    write_timeout: 1s
+    pool_timeout: 2s
+    conn_max_idle_time: 4m
+    max_retries: 2
   max_payload_size: 107374182400
   shared_with_all_users: true
 param_groups:

config/testdata/full.yml

@@ -61,6 +61,19 @@ caches:
       username: chproxy
       password: password
       pool_size: 10
+      # Optional client timeouts. Zero values fall back to the defaults of the
+      # underlying go-redis client.
+      dial_timeout: 2s
+      read_timeout: 1s
+      write_timeout: 1s
+      pool_timeout: 2s
+      # Max idle time of a pooled connection before it's closed. Keep it below
+      # any idle timeout of firewalls/NATs between chproxy and redis.
+      # By default 5m.
+      conn_max_idle_time: 4m
+      # Max number of retries of a failed command. -1 disables retries.
+      # By default 7.
+      max_retries: 2
       addresses:
         - 127.0.0.1:16379
     max_payload_size: 107374182400