diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index 4d38c19d..c40fbc4f 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -1,5 +1,9 @@ # Release Notes +## Unreleased + +- **URL configuration validation:** `ParseURL`, `ParseClusterURL`, and `ParseFailoverURL` now reject negative `dialer_retry_timeout`, `read_buffer_size`, and `write_buffer_size` values. Cluster and failover URLs that previously accepted negative retry timeouts now return an error; replace those values with `0` for no delay, or omit the parameter for the default 100 ms delay. An explicit `dialer_retry_timeout=0` now disables the retry delay ([#4144](https://github.com/redis/go-redis/pull/4144)). + # 9.23.0 (2026-10-05) This is a minor release. It contains everything from 9.23.0-beta.1, so these notes cover the full 9.22.0 → 9.23.0 upgrade. The release adds: diff --git a/options.go b/options.go index 7d8499f5..4c3e8a73 100644 --- a/options.go +++ b/options.go @@ -144,6 +144,9 @@ type Options struct { DialerRetries int // DialerRetryTimeout is the backoff duration between retry attempts. + // Values <= 0 use the default; use DialerRetryBackoff to retry without delay. + // A URL dialer_retry_timeout=0 sets DialerRetryBackoff, which takes precedence + // over this field, including when this field is changed after parsing. // // default: 100 milliseconds DialerRetryTimeout time.Duration @@ -912,6 +915,11 @@ func NewDialer(opt *Options) func(context.Context, string, string) (net.Conn, er // additionally a plain integer as value (i.e. without unit) is interpreted as seconds // - to disable a duration field, use value less than or equal to 0; to use the default // value, leave the value blank or remove the parameter +// - dialer_retry_timeout is an exception: 0 means no retry delay, while negative +// values are rejected. Explicit zero sets DialerRetryBackoff to return 0. +// - read_buffer_size and write_buffer_size must be non-negative; 0 uses the default. +// Invalid negative values are rejected so a configuration typo is not silently +// replaced with an unintended default. // - only the last value is interpreted if a parameter is given multiple times // - fields "network", "addr", "username" and "password" can only be set using other // URL attributes (scheme, host, userinfo, resp.), query parameters using these @@ -1051,6 +1059,45 @@ func (o *queryOptions) int(name string) int { return 0 } +func (o *queryOptions) nonNegativeInt(name string) int { + i := o.int(name) + if i < 0 && o.err == nil { + o.err = fmt.Errorf("redis: invalid %s number: must be non-negative", name) + } + return i +} + +func (o *queryOptions) dialerRetryTimeout() (time.Duration, func(int) time.Duration) { + const name = "dialer_retry_timeout" + s := o.string(name) + if s == "" { + return 0, nil + } + // Like duration, accept plain integers as seconds. ParseDuration also + // rejects overflow rather than letting multiplication wrap to a negative. + if _, err := strconv.Atoi(s); err == nil { + s += "s" + } + d, err := time.ParseDuration(s) + // Check the sign too: ParseDuration rounds negative fractions of a + // nanosecond to zero, but those are still invalid configuration values. + if err == nil && strings.HasPrefix(s, "-") { + err = errors.New("must be non-negative") + } + if err != nil { + if o.err == nil { + o.err = fmt.Errorf("redis: invalid %s duration: %w", name, err) + } + return 0, nil + } + if d == 0 { + // A zero Options field means unset. Use the backoff callback to retain + // an explicit URL zero through client initialization and pool defaults. + return 0, func(int) time.Duration { return 0 } + } + return d, nil +} + func (o *queryOptions) duration(name string) time.Duration { s := o.string(name) if s == "" { @@ -1121,12 +1168,12 @@ func setupConnParams(u *url.URL, o *Options) (*Options, error) { o.MaxRetryBackoff = q.duration("max_retry_backoff") o.DialTimeout = q.duration("dial_timeout") o.DialerRetries = q.int("dialer_retries") - o.DialerRetryTimeout = q.duration("dialer_retry_timeout") + o.DialerRetryTimeout, o.DialerRetryBackoff = q.dialerRetryTimeout() o.ReadTimeout = q.duration("read_timeout") o.WriteTimeout = q.duration("write_timeout") o.ContextTimeoutEnabled = q.bool("context_timeout_enabled") - o.ReadBufferSize = q.int("read_buffer_size") - o.WriteBufferSize = q.int("write_buffer_size") + o.ReadBufferSize = q.nonNegativeInt("read_buffer_size") + o.WriteBufferSize = q.nonNegativeInt("write_buffer_size") o.DisableIdentity = q.bool("disable_identity") o.IdentitySuffix = q.string("identity_suffix") o.PoolFIFO = q.bool("pool_fifo") diff --git a/options_test.go b/options_test.go index 065c0591..1badefd4 100644 --- a/options_test.go +++ b/options_test.go @@ -4,6 +4,7 @@ import ( "crypto/tls" "errors" "net" + "strings" "sync" "testing" "time" @@ -12,6 +13,105 @@ import ( "github.com/redis/go-redis/v9/maintnotifications" ) +func TestParseURLConnectionValidation(t *testing.T) { + parsers := []struct { + name string + parse func(string) (*Options, error) + }{ + {"standalone", func(query string) (*Options, error) { + return ParseURL("redis://localhost:6379?" + query) + }}, + {"cluster", func(query string) (*Options, error) { + o, err := ParseClusterURL("redis://localhost:6379?" + query) + if err != nil { + return nil, err + } + o.init() + return o.clientOptions(), nil + }}, + {"failover", func(query string) (*Options, error) { + o, err := ParseFailoverURL("redis://localhost:6379?master_name=mymaster&" + query) + if err != nil { + return nil, err + } + return o.clientOptions(), nil + }}, + } + for _, parser := range parsers { + t.Run(parser.name, func(t *testing.T) { + t.Run("omitted retry delay", func(t *testing.T) { + o, err := parser.parse("") + if err != nil { + t.Fatal(err) + } + o.init() + if o.DialerRetryTimeout != 100*time.Millisecond || o.DialerRetryBackoff != nil { + t.Fatal("omitted retry delay must retain the default backoff") + } + }) + for _, value := range []string{"", "0", "0s", "1", "250ms", "-1", "-1s", "-1ns", "-0.1ns", "bad", "9223372036854775807"} { + t.Run("retry/"+value, func(t *testing.T) { + o, err := parser.parse("dialer_retry_timeout=" + value) + if strings.HasPrefix(value, "-") || value == "bad" || value == "9223372036854775807" { + if err == nil || !strings.Contains(err.Error(), "dialer_retry_timeout") { + t.Fatalf("expected dialer_retry_timeout error, got %v", err) + } + return + } + if err != nil { + t.Fatal(err) + } + o.init() + want := 100 * time.Millisecond + switch value { + case "0", "0s": + want = 0 + case "1": + want = time.Second + case "250ms": + want = 250 * time.Millisecond + } + got := o.DialerRetryTimeout + if o.DialerRetryBackoff != nil { + got = o.DialerRetryBackoff(0) + } + if got != want { + t.Fatalf("retry delay = %v, want %v", got, want) + } + }) + } + for _, field := range []string{"read_buffer_size", "write_buffer_size"} { + for _, value := range []string{"-1", "bad", "0", "65536"} { + t.Run(field+"/"+value, func(t *testing.T) { + o, err := parser.parse(field + "=" + value) + if value == "-1" || value == "bad" { + if err == nil || !strings.Contains(err.Error(), field) { + t.Fatalf("expected %s error, got %v", field, err) + } + return + } + if err != nil { + t.Fatal(err) + } + o.init() + want := 32768 + if value == "65536" { + want = 65536 + } + got := o.ReadBufferSize + if field == "write_buffer_size" { + got = o.WriteBufferSize + } + if got != want { + t.Fatalf("%s = %d, want %d", field, got, want) + } + }) + } + } + }) + } +} + func TestParseURL(t *testing.T) { cases := []struct { url string diff --git a/osscluster.go b/osscluster.go index 0eac75e8..10668d91 100644 --- a/osscluster.go +++ b/osscluster.go @@ -113,6 +113,9 @@ type ClusterOptions struct { DialerRetries int // DialerRetryTimeout is the backoff duration between retry attempts. + // Values <= 0 use the default; use DialerRetryBackoff to retry without delay. + // A URL dialer_retry_timeout=0 sets DialerRetryBackoff, which takes precedence + // over this field, including when this field is changed after parsing. // // default: 100 milliseconds DialerRetryTimeout time.Duration @@ -330,6 +333,8 @@ func (opt *ClusterOptions) init() { // additionally a plain integer as value (i.e. without unit) is interpreted as seconds // - to disable a duration field, use value less than or equal to 0; to use the default // value, leave the value blank or remove the parameter +// - dialer_retry_timeout is an exception: 0 means no retry delay, while negative +// values are rejected. Explicit zero sets DialerRetryBackoff to return 0. // - only the last value is interpreted if a parameter is given multiple times // - fields "network", "addr", "username" and "password" can only be set using other // URL attributes (scheme, host, userinfo, resp.), query parameters using these @@ -405,12 +410,12 @@ func setupClusterQueryParams(u *url.URL, o *ClusterOptions) (*ClusterOptions, er o.MaxRetryBackoff = q.duration("max_retry_backoff") o.DialTimeout = q.duration("dial_timeout") o.DialerRetries = q.int("dialer_retries") - o.DialerRetryTimeout = q.duration("dialer_retry_timeout") + o.DialerRetryTimeout, o.DialerRetryBackoff = q.dialerRetryTimeout() o.ReadTimeout = q.duration("read_timeout") o.WriteTimeout = q.duration("write_timeout") o.ContextTimeoutEnabled = q.bool("context_timeout_enabled") - o.ReadBufferSize = q.int("read_buffer_size") - o.WriteBufferSize = q.int("write_buffer_size") + o.ReadBufferSize = q.nonNegativeInt("read_buffer_size") + o.WriteBufferSize = q.nonNegativeInt("write_buffer_size") o.DisableIdentity = q.bool("disable_identity") o.IdentitySuffix = q.string("identity_suffix") o.PoolFIFO = q.bool("pool_fifo") diff --git a/sentinel.go b/sentinel.go index a2f00a95..c810ef93 100644 --- a/sentinel.go +++ b/sentinel.go @@ -101,6 +101,9 @@ type FailoverOptions struct { DialerRetries int // DialerRetryTimeout is the backoff duration between retry attempts. + // Values <= 0 use the default; use DialerRetryBackoff to retry without delay. + // A URL dialer_retry_timeout=0 sets DialerRetryBackoff, which takes precedence + // over this field, including when this field is changed after parsing. // // default: 100 milliseconds DialerRetryTimeout time.Duration @@ -402,6 +405,8 @@ func (opt *FailoverOptions) clusterOptions() *ClusterOptions { // additionally a plain integer as value (i.e. without unit) is interpreted as seconds // - to disable a duration field, use value less than or equal to 0; to use the default // value, leave the value blank or remove the parameter +// - dialer_retry_timeout is an exception: 0 means no retry delay, while negative +// values are rejected. Explicit zero sets DialerRetryBackoff to return 0. // - only the last value is interpreted if a parameter is given multiple times // - fields "network", "addr", "sentinel_username" and "sentinel_password" can only be set using other // URL attributes (scheme, host, userinfo, resp.), query parameters using these @@ -480,12 +485,12 @@ func setupFailoverConnParams(u *url.URL, o *FailoverOptions) (*FailoverOptions, o.MaxRetryBackoff = q.duration("max_retry_backoff") o.DialTimeout = q.duration("dial_timeout") o.DialerRetries = q.int("dialer_retries") - o.DialerRetryTimeout = q.duration("dialer_retry_timeout") + o.DialerRetryTimeout, o.DialerRetryBackoff = q.dialerRetryTimeout() o.ReadTimeout = q.duration("read_timeout") o.WriteTimeout = q.duration("write_timeout") o.ContextTimeoutEnabled = q.bool("context_timeout_enabled") - o.ReadBufferSize = q.int("read_buffer_size") - o.WriteBufferSize = q.int("write_buffer_size") + o.ReadBufferSize = q.nonNegativeInt("read_buffer_size") + o.WriteBufferSize = q.nonNegativeInt("write_buffer_size") o.PoolFIFO = q.bool("pool_fifo") o.PoolSize = q.int("pool_size") o.MaxConcurrentDials = q.int("max_concurrent_dials")