From c1aa637a66cea701685689d3c28ba336d9ebe10a Mon Sep 17 00:00:00 2001 From: notdu Date: Sat, 6 Dec 2025 18:52:45 +0700 Subject: [PATCH 1/4] feat: Add Pool On-Empty Behavior Configuration for Redis Connections Signed-off-by: notdu --- README.md | 13 +++ src/redis/cache_impl.go | 6 +- src/redis/driver_impl.go | 20 ++++- src/settings/settings.go | 21 +++++ src/settings/settings_test.go | 114 ++++++++++++++++++++++++ src/srv/srv_test.go | 6 +- test/redis/bench_test.go | 2 +- test/redis/driver_impl_test.go | 154 ++++++++++++++++++++++++++++++++- 8 files changed, 328 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 3fb807767..4e745db0c 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,7 @@ - [Redis](#redis) - [Redis type](#redis-type) - [Connection Timeout](#connection-timeout) + - [Pool On-Empty Behavior](#pool-on-empty-behavior) - [Pipelining](#pipelining) - [One Redis Instance](#one-redis-instance) - [Two Redis Instances](#two-redis-instances) @@ -1281,6 +1282,18 @@ Connection timeout controls the maximum duration for Redis connection establishm 1. `REDIS_TIMEOUT`: sets the timeout for Redis connection and I/O operations. Default: `10s` 1. `REDIS_PERSECOND_TIMEOUT`: sets the timeout for per-second Redis connection and I/O operations. Default: `10s` +## Pool On-Empty Behavior + +Controls what happens when all connections in the Redis pool are in use and a new request arrives. + +1. `REDIS_POOL_ON_EMPTY_BEHAVIOR`: controls what happens when the pool is empty. Default: `""` (radix default: create after 1s) + - `ERROR`: return an error after waiting for the specified duration. This enforces a strict pool size limit. + - `CREATE`: create a new overflow connection after waiting for the specified duration. This is the [default radix behavior](https://github.com/mediocregopher/radix/blob/v3.8.1/pool.go#L291-L312). + - `WAIT`: block until a connection becomes available. This enforces a strict pool size limit but may cause goroutine buildup. +1. `REDIS_POOL_ON_EMPTY_WAIT_DURATION`: the duration to wait before taking the configured action. Default: `0` (immediate) +1. `REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR`: same as above for per-second Redis pool. Default: `""` +1. `REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION`: same as above for per-second Redis pool. Default: `0` + ## Pipelining By default, for each request, ratelimit will pick up a connection from pool, write multiple redis commands in a single write then reads their responses in a single read. This reduces network delay. diff --git a/src/redis/cache_impl.go b/src/redis/cache_impl.go index 6a5128e5f..43a0b2f04 100644 --- a/src/redis/cache_impl.go +++ b/src/redis/cache_impl.go @@ -18,12 +18,14 @@ func NewRateLimiterCacheImplFromSettings(s settings.Settings, localCache *freeca var perSecondPool Client if s.RedisPerSecond { perSecondPool = NewClientImpl(srv.Scope().Scope("redis_per_second_pool"), s.RedisPerSecondTls, s.RedisPerSecondAuth, s.RedisPerSecondSocketType, - s.RedisPerSecondType, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize, s.RedisPerSecondPipelineWindow, s.RedisPerSecondPipelineLimit, s.RedisTlsConfig, s.RedisHealthCheckActiveConnection, srv, s.RedisPerSecondTimeout) + s.RedisPerSecondType, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize, s.RedisPerSecondPipelineWindow, s.RedisPerSecondPipelineLimit, s.RedisTlsConfig, s.RedisHealthCheckActiveConnection, srv, s.RedisPerSecondTimeout, + s.RedisPerSecondPoolOnEmptyBehavior, s.RedisPerSecondPoolOnEmptyWaitDuration) closer.Closers = append(closer.Closers, perSecondPool) } otherPool := NewClientImpl(srv.Scope().Scope("redis_pool"), s.RedisTls, s.RedisAuth, s.RedisSocketType, s.RedisType, s.RedisUrl, s.RedisPoolSize, - s.RedisPipelineWindow, s.RedisPipelineLimit, s.RedisTlsConfig, s.RedisHealthCheckActiveConnection, srv, s.RedisTimeout) + s.RedisPipelineWindow, s.RedisPipelineLimit, s.RedisTlsConfig, s.RedisHealthCheckActiveConnection, srv, s.RedisTimeout, + s.RedisPoolOnEmptyBehavior, s.RedisPoolOnEmptyWaitDuration) closer.Closers = append(closer.Closers, otherPool) return NewFixedRateLimitCacheImpl( diff --git a/src/redis/driver_impl.go b/src/redis/driver_impl.go index 98c9f6e1f..946a9f165 100644 --- a/src/redis/driver_impl.go +++ b/src/redis/driver_impl.go @@ -72,7 +72,7 @@ func checkError(err error) { func NewClientImpl(scope stats.Scope, useTls bool, auth, redisSocketType, redisType, url string, poolSize int, pipelineWindow time.Duration, pipelineLimit int, tlsConfig *tls.Config, healthCheckActiveConnection bool, srv server.Server, - timeout time.Duration, + timeout time.Duration, poolOnEmptyBehavior string, poolOnEmptyWaitDuration time.Duration, ) Client { maskedUrl := utils.MaskCredentialsInUrl(url) logger.Warnf("connecting to redis on %s with pool size %d", maskedUrl, poolSize) @@ -112,6 +112,24 @@ func NewClientImpl(scope stats.Scope, useTls bool, auth, redisSocketType, redisT } logger.Debugf("Implicit pipelining enabled: %v", implicitPipelining) + // Configure pool on-empty behavior to prevent connection storms during Redis failures + switch strings.ToUpper(poolOnEmptyBehavior) { + case "WAIT": + opts = append(opts, radix.PoolOnEmptyWait()) + logger.Warnf("Redis pool %s: on-empty=WAIT (block until connection available)", maskedUrl) + case "CREATE": + opts = append(opts, radix.PoolOnEmptyCreateAfter(poolOnEmptyWaitDuration)) + logger.Warnf("Redis pool %s: on-empty=CREATE after %v", maskedUrl, poolOnEmptyWaitDuration) + case "ERROR": + opts = append(opts, radix.PoolOnEmptyErrAfter(poolOnEmptyWaitDuration)) + logger.Warnf("Redis pool %s: on-empty=ERROR after %v (fail-fast)", maskedUrl, poolOnEmptyWaitDuration) + default: + // Empty string = use radix default (PoolOnEmptyCreateAfter(1s)) + if poolOnEmptyBehavior != "" { + logger.Warnf("Redis pool %s: unknown on-empty behavior '%s', using default (CREATE after 1s)", maskedUrl, poolOnEmptyBehavior) + } + } + poolFunc := func(network, addr string) (radix.Client, error) { return radix.NewPool(network, addr, poolSize, opts...) } diff --git a/src/settings/settings.go b/src/settings/settings.go index 0eb53f814..0c13f1946 100644 --- a/src/settings/settings.go +++ b/src/settings/settings.go @@ -163,6 +163,27 @@ type Settings struct { RedisTimeout time.Duration `envconfig:"REDIS_TIMEOUT" default:"10s"` // RedisPerSecondTimeout sets the timeout for per-second Redis connection and I/O operations. RedisPerSecondTimeout time.Duration `envconfig:"REDIS_PERSECOND_TIMEOUT" default:"10s"` + + // RedisPoolOnEmptyBehavior controls what happens when Redis connection pool is empty. + // This setting helps prevent connection storms during Redis failures. + // Possible values: + // - "" (empty): Use radix default (create new connection after 1s wait) + // - "WAIT": Block until a connection is available + // - "CREATE": Create a new connection after RedisPoolOnEmptyWaitDuration + // - "ERROR": Return error after RedisPoolOnEmptyWaitDuration + RedisPoolOnEmptyBehavior string `envconfig:"REDIS_POOL_ON_EMPTY_BEHAVIOR" default:""` + // RedisPoolOnEmptyWaitDuration is the wait duration before taking action when pool is empty. + // Only applicable when RedisPoolOnEmptyBehavior is "CREATE" or "ERROR". + // Default 0 means immediate action. + RedisPoolOnEmptyWaitDuration time.Duration `envconfig:"REDIS_POOL_ON_EMPTY_WAIT_DURATION" default:"0"` + + // RedisPerSecondPoolOnEmptyBehavior controls pool-empty behavior for per-second Redis. + // See RedisPoolOnEmptyBehavior for possible values and details. + RedisPerSecondPoolOnEmptyBehavior string `envconfig:"REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR" default:""` + // RedisPerSecondPoolOnEmptyWaitDuration is the wait duration for per-second Redis pool. + // See RedisPoolOnEmptyWaitDuration for details. + RedisPerSecondPoolOnEmptyWaitDuration time.Duration `envconfig:"REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION" default:"0"` + // Memcache settings MemcacheHostPort []string `envconfig:"MEMCACHE_HOST_PORT" default:""` // MemcacheMaxIdleConns sets the maximum number of idle TCP connections per memcached node. diff --git a/src/settings/settings_test.go b/src/settings/settings_test.go index 0a391b78d..fa272b30b 100644 --- a/src/settings/settings_test.go +++ b/src/settings/settings_test.go @@ -1,7 +1,9 @@ package settings import ( + "os" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -11,3 +13,115 @@ func TestSettingsTlsConfigUnmodified(t *testing.T) { assert.NotNil(t, settings.RedisTlsConfig) assert.Nil(t, settings.RedisTlsConfig.RootCAs) } + +// Tests for RedisPoolOnEmptyBehavior +func TestRedisPoolOnEmptyBehavior_Default(t *testing.T) { + os.Unsetenv("REDIS_POOL_ON_EMPTY_BEHAVIOR") + os.Unsetenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION") + + settings := NewSettings() + + assert.Equal(t, "", settings.RedisPoolOnEmptyBehavior) + assert.Equal(t, time.Duration(0), settings.RedisPoolOnEmptyWaitDuration) +} + +func TestRedisPoolOnEmptyBehavior_Error(t *testing.T) { + os.Setenv("REDIS_POOL_ON_EMPTY_BEHAVIOR", "ERROR") + os.Setenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION", "0") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_BEHAVIOR") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION") + + settings := NewSettings() + + assert.Equal(t, "ERROR", settings.RedisPoolOnEmptyBehavior) + assert.Equal(t, time.Duration(0), settings.RedisPoolOnEmptyWaitDuration) +} + +func TestRedisPoolOnEmptyBehavior_ErrorWithDuration(t *testing.T) { + os.Setenv("REDIS_POOL_ON_EMPTY_BEHAVIOR", "ERROR") + os.Setenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION", "100ms") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_BEHAVIOR") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION") + + settings := NewSettings() + + assert.Equal(t, "ERROR", settings.RedisPoolOnEmptyBehavior) + assert.Equal(t, 100*time.Millisecond, settings.RedisPoolOnEmptyWaitDuration) +} + +func TestRedisPoolOnEmptyBehavior_Create(t *testing.T) { + os.Setenv("REDIS_POOL_ON_EMPTY_BEHAVIOR", "CREATE") + os.Setenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION", "500ms") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_BEHAVIOR") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION") + + settings := NewSettings() + + assert.Equal(t, "CREATE", settings.RedisPoolOnEmptyBehavior) + assert.Equal(t, 500*time.Millisecond, settings.RedisPoolOnEmptyWaitDuration) +} + +func TestRedisPoolOnEmptyBehavior_Wait(t *testing.T) { + os.Setenv("REDIS_POOL_ON_EMPTY_BEHAVIOR", "WAIT") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_BEHAVIOR") + + settings := NewSettings() + + assert.Equal(t, "WAIT", settings.RedisPoolOnEmptyBehavior) +} + +func TestRedisPoolOnEmptyBehavior_CaseInsensitive(t *testing.T) { + // Test that lowercase values work (processing is done in driver_impl.go) + os.Setenv("REDIS_POOL_ON_EMPTY_BEHAVIOR", "error") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_BEHAVIOR") + + settings := NewSettings() + + // Setting stores as-is, case conversion happens in driver_impl.go + assert.Equal(t, "error", settings.RedisPoolOnEmptyBehavior) +} + +// Tests for RedisPerSecondPoolOnEmptyBehavior +func TestRedisPerSecondPoolOnEmptyBehavior_Default(t *testing.T) { + os.Unsetenv("REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR") + os.Unsetenv("REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION") + + settings := NewSettings() + + assert.Equal(t, "", settings.RedisPerSecondPoolOnEmptyBehavior) + assert.Equal(t, time.Duration(0), settings.RedisPerSecondPoolOnEmptyWaitDuration) +} + +func TestRedisPerSecondPoolOnEmptyBehavior_Error(t *testing.T) { + os.Setenv("REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR", "ERROR") + os.Setenv("REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION", "50ms") + defer os.Unsetenv("REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR") + defer os.Unsetenv("REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION") + + settings := NewSettings() + + assert.Equal(t, "ERROR", settings.RedisPerSecondPoolOnEmptyBehavior) + assert.Equal(t, 50*time.Millisecond, settings.RedisPerSecondPoolOnEmptyWaitDuration) +} + +// Test both pools can be configured independently +func TestRedisPoolOnEmptyBehavior_IndependentConfiguration(t *testing.T) { + os.Setenv("REDIS_POOL_ON_EMPTY_BEHAVIOR", "ERROR") + os.Setenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION", "0") + os.Setenv("REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR", "CREATE") + os.Setenv("REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION", "100ms") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_BEHAVIOR") + defer os.Unsetenv("REDIS_POOL_ON_EMPTY_WAIT_DURATION") + defer os.Unsetenv("REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR") + defer os.Unsetenv("REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION") + + settings := NewSettings() + + // Main pool configured for fail-fast + assert.Equal(t, "ERROR", settings.RedisPoolOnEmptyBehavior) + assert.Equal(t, time.Duration(0), settings.RedisPoolOnEmptyWaitDuration) + + // Per-second pool configured differently + assert.Equal(t, "CREATE", settings.RedisPerSecondPoolOnEmptyBehavior) + assert.Equal(t, 100*time.Millisecond, settings.RedisPerSecondPoolOnEmptyWaitDuration) +} diff --git a/src/srv/srv_test.go b/src/srv/srv_test.go index 64a36682f..3e4d03255 100644 --- a/src/srv/srv_test.go +++ b/src/srv/srv_test.go @@ -8,7 +8,11 @@ import ( ) func mockAddrsLookup(service, proto, name string) (cname string, addrs []*net.SRV, err error) { - return "ignored", []*net.SRV{{"z", 1, 0, 0}, {"z", 0, 0, 0}, {"a", 9001, 0, 0}}, nil + return "ignored", []*net.SRV{ + {Target: "z", Port: 1, Priority: 0, Weight: 0}, + {Target: "z", Port: 0, Priority: 0, Weight: 0}, + {Target: "a", Port: 9001, Priority: 0, Weight: 0}, + }, nil } func TestLookupServerStringsFromSrvReturnsServersSorted(t *testing.T) { diff --git a/test/redis/bench_test.go b/test/redis/bench_test.go index 0f654e2d6..2a3aad3ad 100644 --- a/test/redis/bench_test.go +++ b/test/redis/bench_test.go @@ -44,7 +44,7 @@ func BenchmarkParallelDoLimit(b *testing.B) { return func(b *testing.B) { statsStore := gostats.NewStore(gostats.NewNullSink(), false) sm := stats.NewMockStatManager(statsStore) - client := redis.NewClientImpl(statsStore, false, "", "tcp", "single", "127.0.0.1:6379", poolSize, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second) + client := redis.NewClientImpl(statsStore, false, "", "tcp", "single", "127.0.0.1:6379", poolSize, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second, "", 0) defer client.Close() cache := redis.NewFixedRateLimitCacheImpl(client, nil, utils.NewTimeSourceImpl(), rand.New(utils.NewLockedSource(time.Now().Unix())), 10, nil, 0.8, "", sm, true) diff --git a/test/redis/driver_impl_test.go b/test/redis/driver_impl_test.go index 3ca96b671..ae4ad7f36 100644 --- a/test/redis/driver_impl_test.go +++ b/test/redis/driver_impl_test.go @@ -38,7 +38,7 @@ func testNewClientImpl(t *testing.T, pipelineWindow time.Duration, pipelineLimit statsStore := stats.NewStore(stats.NewNullSink(), false) mkRedisClient := func(auth, addr string) redis.Client { - return redis.NewClientImpl(statsStore, false, auth, "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second) + return redis.NewClientImpl(statsStore, false, auth, "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second, "", 0) } t.Run("connection refused", func(t *testing.T) { @@ -131,7 +131,7 @@ func TestDoCmd(t *testing.T) { statsStore := stats.NewStore(stats.NewNullSink(), false) mkRedisClient := func(addr string) redis.Client { - return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, 0, 0, nil, false, nil, 10*time.Second) + return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, 0, 0, nil, false, nil, 10*time.Second, "", 0) } t.Run("SETGET ok", func(t *testing.T) { @@ -176,7 +176,7 @@ func testPipeDo(t *testing.T, pipelineWindow time.Duration, pipelineLimit int) f statsStore := stats.NewStore(stats.NewNullSink(), false) mkRedisClient := func(addr string) redis.Client { - return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second) + return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, pipelineWindow, pipelineLimit, nil, false, nil, 10*time.Second, "", 0) } t.Run("SETGET ok", func(t *testing.T) { @@ -231,3 +231,151 @@ func TestPipeDo(t *testing.T) { t.Run("ImplicitPipeliningEnabled", testPipeDo(t, 10*time.Millisecond, 2)) t.Run("ImplicitPipeliningDisabled", testPipeDo(t, 0, 0)) } + +// Tests for pool on-empty behavior +func TestPoolOnEmptyBehavior(t *testing.T) { + statsStore := stats.NewStore(stats.NewNullSink(), false) + + // Helper to create client with specific on-empty behavior + mkRedisClientWithBehavior := func(addr, behavior string, waitDuration time.Duration) redis.Client { + return redis.NewClientImpl(statsStore, false, "", "tcp", "single", addr, 1, 0, 0, nil, false, nil, 10*time.Second, behavior, waitDuration) + } + + t.Run("default behavior (empty string)", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "", 0) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "foo", "bar")) + assert.Nil(t, client.DoCmd(&res, "GET", "foo")) + assert.Equal(t, "bar", res) + }) + + t.Run("ERROR behavior", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "ERROR", 0) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "test", "value")) + assert.Nil(t, client.DoCmd(&res, "GET", "test")) + assert.Equal(t, "value", res) + }) + + t.Run("ERROR behavior with wait duration", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "ERROR", 100*time.Millisecond) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "test2", "value2")) + assert.Nil(t, client.DoCmd(&res, "GET", "test2")) + assert.Equal(t, "value2", res) + }) + + t.Run("CREATE behavior", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "CREATE", 0) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "test3", "value3")) + assert.Nil(t, client.DoCmd(&res, "GET", "test3")) + assert.Equal(t, "value3", res) + }) + + t.Run("CREATE behavior with wait duration", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "CREATE", 500*time.Millisecond) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "test4", "value4")) + assert.Nil(t, client.DoCmd(&res, "GET", "test4")) + assert.Equal(t, "value4", res) + }) + + t.Run("WAIT behavior", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "WAIT", 0) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "test5", "value5")) + assert.Nil(t, client.DoCmd(&res, "GET", "test5")) + assert.Equal(t, "value5", res) + }) + + t.Run("case insensitive behavior", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + // Test lowercase + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "error", 0) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "test6", "value6")) + assert.Nil(t, client.DoCmd(&res, "GET", "test6")) + assert.Equal(t, "value6", res) + }) + + t.Run("unknown behavior falls back to default", func(t *testing.T) { + redisSrv := mustNewRedisServer() + defer redisSrv.Close() + + // Unknown behavior should not panic, just log warning and use default + var client redis.Client + assert.NotPanics(t, func() { + client = mkRedisClientWithBehavior(redisSrv.Addr(), "UNKNOWN_BEHAVIOR", 0) + }) + assert.NotNil(t, client) + + // Verify client works + var res string + assert.Nil(t, client.DoCmd(nil, "SET", "test7", "value7")) + assert.Nil(t, client.DoCmd(&res, "GET", "test7")) + assert.Equal(t, "value7", res) + }) +} From 779055d6560fc2e53b11a66ca41b9ef9d59939fa Mon Sep 17 00:00:00 2001 From: notdu Date: Sat, 6 Dec 2025 22:59:04 +0700 Subject: [PATCH 2/4] update Signed-off-by: notdu --- README.md | 30 +++++++++++++++++++----------- src/redis/driver_impl.go | 1 - 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 4e745db0c..921ba9514 100644 --- a/README.md +++ b/README.md @@ -61,9 +61,11 @@ - [Local Cache](#local-cache) - [Redis](#redis) - [Redis type](#redis-type) - - [Connection Timeout](#connection-timeout) - - [Pool On-Empty Behavior](#pool-on-empty-behavior) - - [Pipelining](#pipelining) + - [Connection Pool Settings](#connection-pool-settings) + - [Pool Size](#pool-size) + - [Connection Timeout](#connection-timeout) + - [Pool On-Empty Behavior](#pool-on-empty-behavior) + - [Pipelining](#pipelining) - [One Redis Instance](#one-redis-instance) - [Two Redis Instances](#two-redis-instances) - [Health Checking for Redis Active Connection](#health-checking-for-redis-active-connection) @@ -1275,26 +1277,32 @@ The deployment type can be specified with the `REDIS_TYPE` / `REDIS_PERSECOND_TY 1. "sentinel": A comma separated list with the first string as the master name of the sentinel cluster followed by hostname:port pairs. The list size should be >= 2. The first item is the name of the master and the rest are the sentinels. 1. "cluster": A comma separated list of hostname:port pairs with all the nodes in the cluster. -## Connection Timeout +## Connection Pool Settings +### Pool Size -Connection timeout controls the maximum duration for Redis connection establishment, read operations, and write operations. +1. `REDIS_POOL_SIZE`: the number of connections to keep in the pool. Default: `10` +1. `REDIS_PERSECOND_POOL_SIZE`: pool size for per-second Redis. Default: `10` + +### Connection Timeout + +Controls the maximum duration for Redis connection establishment, read operations, and write operations. 1. `REDIS_TIMEOUT`: sets the timeout for Redis connection and I/O operations. Default: `10s` 1. `REDIS_PERSECOND_TIMEOUT`: sets the timeout for per-second Redis connection and I/O operations. Default: `10s` -## Pool On-Empty Behavior +### Pool On-Empty Behavior -Controls what happens when all connections in the Redis pool are in use and a new request arrives. +Controls what happens when all connections in the pool are in use and a new request arrives. 1. `REDIS_POOL_ON_EMPTY_BEHAVIOR`: controls what happens when the pool is empty. Default: `""` (radix default: create after 1s) - - `ERROR`: return an error after waiting for the specified duration. This enforces a strict pool size limit. - - `CREATE`: create a new overflow connection after waiting for the specified duration. This is the [default radix behavior](https://github.com/mediocregopher/radix/blob/v3.8.1/pool.go#L291-L312). + - `ERROR`: return an error after waiting for `REDIS_POOL_ON_EMPTY_WAIT_DURATION`. This enforces a strict pool size limit and is recommended for production to fail fast during Redis outages. + - `CREATE`: create a new overflow connection after waiting for `REDIS_POOL_ON_EMPTY_WAIT_DURATION`. This is the [default radix behavior](https://github.com/mediocregopher/radix/blob/v3.8.1/pool.go#L291-L312). - `WAIT`: block until a connection becomes available. This enforces a strict pool size limit but may cause goroutine buildup. -1. `REDIS_POOL_ON_EMPTY_WAIT_DURATION`: the duration to wait before taking the configured action. Default: `0` (immediate) +1. `REDIS_POOL_ON_EMPTY_WAIT_DURATION`: the duration to wait before taking the configured action (`ERROR` or `CREATE`). Default: `0` (immediate) 1. `REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR`: same as above for per-second Redis pool. Default: `""` 1. `REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION`: same as above for per-second Redis pool. Default: `0` -## Pipelining +### Pipelining By default, for each request, ratelimit will pick up a connection from pool, write multiple redis commands in a single write then reads their responses in a single read. This reduces network delay. diff --git a/src/redis/driver_impl.go b/src/redis/driver_impl.go index 946a9f165..59e51ffc4 100644 --- a/src/redis/driver_impl.go +++ b/src/redis/driver_impl.go @@ -112,7 +112,6 @@ func NewClientImpl(scope stats.Scope, useTls bool, auth, redisSocketType, redisT } logger.Debugf("Implicit pipelining enabled: %v", implicitPipelining) - // Configure pool on-empty behavior to prevent connection storms during Redis failures switch strings.ToUpper(poolOnEmptyBehavior) { case "WAIT": opts = append(opts, radix.PoolOnEmptyWait()) From e38a8d8183753d6edfe4a7085ee091d733aa0a0d Mon Sep 17 00:00:00 2001 From: notdu Date: Sat, 6 Dec 2025 23:04:33 +0700 Subject: [PATCH 3/4] update Signed-off-by: notdu --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 921ba9514..31eddf540 100644 --- a/README.md +++ b/README.md @@ -1278,6 +1278,7 @@ The deployment type can be specified with the `REDIS_TYPE` / `REDIS_PERSECOND_TY 1. "cluster": A comma separated list of hostname:port pairs with all the nodes in the cluster. ## Connection Pool Settings + ### Pool Size 1. `REDIS_POOL_SIZE`: the number of connections to keep in the pool. Default: `10` From 7c20f16c994bb3c1918394929c02e66c284cd557 Mon Sep 17 00:00:00 2001 From: notdu Date: Sun, 7 Dec 2025 14:51:26 +0700 Subject: [PATCH 4/4] update Signed-off-by: notdu --- README.md | 10 +++++----- src/redis/driver_impl.go | 6 ++---- src/settings/settings.go | 14 ++++++-------- src/settings/settings_test.go | 8 ++++---- 4 files changed, 17 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 31eddf540..7fafd26b5 100644 --- a/README.md +++ b/README.md @@ -1295,13 +1295,13 @@ Controls the maximum duration for Redis connection establishment, read operation Controls what happens when all connections in the pool are in use and a new request arrives. -1. `REDIS_POOL_ON_EMPTY_BEHAVIOR`: controls what happens when the pool is empty. Default: `""` (radix default: create after 1s) - - `ERROR`: return an error after waiting for `REDIS_POOL_ON_EMPTY_WAIT_DURATION`. This enforces a strict pool size limit and is recommended for production to fail fast during Redis outages. +1. `REDIS_POOL_ON_EMPTY_BEHAVIOR`: controls what happens when the pool is empty. Default: `CREATE` - `CREATE`: create a new overflow connection after waiting for `REDIS_POOL_ON_EMPTY_WAIT_DURATION`. This is the [default radix behavior](https://github.com/mediocregopher/radix/blob/v3.8.1/pool.go#L291-L312). + - `ERROR`: return an error after waiting for `REDIS_POOL_ON_EMPTY_WAIT_DURATION`. This enforces a strict pool size limit. - `WAIT`: block until a connection becomes available. This enforces a strict pool size limit but may cause goroutine buildup. -1. `REDIS_POOL_ON_EMPTY_WAIT_DURATION`: the duration to wait before taking the configured action (`ERROR` or `CREATE`). Default: `0` (immediate) -1. `REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR`: same as above for per-second Redis pool. Default: `""` -1. `REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION`: same as above for per-second Redis pool. Default: `0` +1. `REDIS_POOL_ON_EMPTY_WAIT_DURATION`: the duration to wait before taking the configured action (`CREATE` or `ERROR`). Default: `1s` +1. `REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR`: same as above for per-second Redis pool. Default: `CREATE` +1. `REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION`: same as above for per-second Redis pool. Default: `1s` ### Pipelining diff --git a/src/redis/driver_impl.go b/src/redis/driver_impl.go index 59e51ffc4..00a8074af 100644 --- a/src/redis/driver_impl.go +++ b/src/redis/driver_impl.go @@ -123,10 +123,8 @@ func NewClientImpl(scope stats.Scope, useTls bool, auth, redisSocketType, redisT opts = append(opts, radix.PoolOnEmptyErrAfter(poolOnEmptyWaitDuration)) logger.Warnf("Redis pool %s: on-empty=ERROR after %v (fail-fast)", maskedUrl, poolOnEmptyWaitDuration) default: - // Empty string = use radix default (PoolOnEmptyCreateAfter(1s)) - if poolOnEmptyBehavior != "" { - logger.Warnf("Redis pool %s: unknown on-empty behavior '%s', using default (CREATE after 1s)", maskedUrl, poolOnEmptyBehavior) - } + logger.Warnf("Redis pool %s: invalid on-empty behavior '%s', using default CREATE after %v", maskedUrl, poolOnEmptyBehavior, poolOnEmptyWaitDuration) + opts = append(opts, radix.PoolOnEmptyCreateAfter(poolOnEmptyWaitDuration)) } poolFunc := func(network, addr string) (radix.Client, error) { diff --git a/src/settings/settings.go b/src/settings/settings.go index 0c13f1946..2c14a478e 100644 --- a/src/settings/settings.go +++ b/src/settings/settings.go @@ -167,22 +167,20 @@ type Settings struct { // RedisPoolOnEmptyBehavior controls what happens when Redis connection pool is empty. // This setting helps prevent connection storms during Redis failures. // Possible values: - // - "" (empty): Use radix default (create new connection after 1s wait) - // - "WAIT": Block until a connection is available - // - "CREATE": Create a new connection after RedisPoolOnEmptyWaitDuration + // - "CREATE": Create a new connection after RedisPoolOnEmptyWaitDuration (default) // - "ERROR": Return error after RedisPoolOnEmptyWaitDuration - RedisPoolOnEmptyBehavior string `envconfig:"REDIS_POOL_ON_EMPTY_BEHAVIOR" default:""` + // - "WAIT": Block until a connection is available + RedisPoolOnEmptyBehavior string `envconfig:"REDIS_POOL_ON_EMPTY_BEHAVIOR" default:"CREATE"` // RedisPoolOnEmptyWaitDuration is the wait duration before taking action when pool is empty. // Only applicable when RedisPoolOnEmptyBehavior is "CREATE" or "ERROR". - // Default 0 means immediate action. - RedisPoolOnEmptyWaitDuration time.Duration `envconfig:"REDIS_POOL_ON_EMPTY_WAIT_DURATION" default:"0"` + RedisPoolOnEmptyWaitDuration time.Duration `envconfig:"REDIS_POOL_ON_EMPTY_WAIT_DURATION" default:"1s"` // RedisPerSecondPoolOnEmptyBehavior controls pool-empty behavior for per-second Redis. // See RedisPoolOnEmptyBehavior for possible values and details. - RedisPerSecondPoolOnEmptyBehavior string `envconfig:"REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR" default:""` + RedisPerSecondPoolOnEmptyBehavior string `envconfig:"REDIS_PERSECOND_POOL_ON_EMPTY_BEHAVIOR" default:"CREATE"` // RedisPerSecondPoolOnEmptyWaitDuration is the wait duration for per-second Redis pool. // See RedisPoolOnEmptyWaitDuration for details. - RedisPerSecondPoolOnEmptyWaitDuration time.Duration `envconfig:"REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION" default:"0"` + RedisPerSecondPoolOnEmptyWaitDuration time.Duration `envconfig:"REDIS_PERSECOND_POOL_ON_EMPTY_WAIT_DURATION" default:"1s"` // Memcache settings MemcacheHostPort []string `envconfig:"MEMCACHE_HOST_PORT" default:""` diff --git a/src/settings/settings_test.go b/src/settings/settings_test.go index fa272b30b..e706196ee 100644 --- a/src/settings/settings_test.go +++ b/src/settings/settings_test.go @@ -21,8 +21,8 @@ func TestRedisPoolOnEmptyBehavior_Default(t *testing.T) { settings := NewSettings() - assert.Equal(t, "", settings.RedisPoolOnEmptyBehavior) - assert.Equal(t, time.Duration(0), settings.RedisPoolOnEmptyWaitDuration) + assert.Equal(t, "CREATE", settings.RedisPoolOnEmptyBehavior) + assert.Equal(t, 1*time.Second, settings.RedisPoolOnEmptyWaitDuration) } func TestRedisPoolOnEmptyBehavior_Error(t *testing.T) { @@ -88,8 +88,8 @@ func TestRedisPerSecondPoolOnEmptyBehavior_Default(t *testing.T) { settings := NewSettings() - assert.Equal(t, "", settings.RedisPerSecondPoolOnEmptyBehavior) - assert.Equal(t, time.Duration(0), settings.RedisPerSecondPoolOnEmptyWaitDuration) + assert.Equal(t, "CREATE", settings.RedisPerSecondPoolOnEmptyBehavior) + assert.Equal(t, 1*time.Second, settings.RedisPerSecondPoolOnEmptyWaitDuration) } func TestRedisPerSecondPoolOnEmptyBehavior_Error(t *testing.T) {