From 61ab5741bc6064ea9cf72a622f999fb18c006c53 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Tue, 18 Nov 2025 14:39:36 +0200 Subject: [PATCH 01/12] typed errors --- error.go | 111 +++++++--- error_wrapping_test.go | 311 +++++++++++++++++++++++++++ internal/proto/reader.go | 5 +- internal/proto/redis_errors.go | 287 +++++++++++++++++++++++++ internal/proto/redis_errors_test.go | 319 ++++++++++++++++++++++++++++ 5 files changed, 1004 insertions(+), 29 deletions(-) create mode 100644 error_wrapping_test.go create mode 100644 internal/proto/redis_errors.go create mode 100644 internal/proto/redis_errors_test.go diff --git a/error.go b/error.go index 7273313b53..33c0651200 100644 --- a/error.go +++ b/error.go @@ -78,23 +78,23 @@ func shouldRetry(err error, retryTimeout bool) bool { return true } - s := err.Error() - if s == "ERR max number of clients reached" { + // Check for typed Redis errors using errors.As (works with wrapped errors) + if proto.IsMaxClientsError(err) { return true } - if strings.HasPrefix(s, "LOADING ") { + if proto.IsLoadingError(err) { return true } - if strings.HasPrefix(s, "READONLY ") { + if proto.IsReadOnlyError(err) { return true } - if strings.HasPrefix(s, "MASTERDOWN ") { + if proto.IsMasterDownError(err) { return true } - if strings.HasPrefix(s, "CLUSTERDOWN ") { + if proto.IsClusterDownError(err) { return true } - if strings.HasPrefix(s, "TRYAGAIN ") { + if proto.IsTryAgainError(err) { return true } @@ -146,40 +146,97 @@ func isMovedError(err error) (moved bool, ask bool, addr string) { return } - s := err.Error() - switch { - case strings.HasPrefix(s, "MOVED "): - moved = true - case strings.HasPrefix(s, "ASK "): - ask = true - default: - return + // Check for typed MovedError + if movedErr, ok := proto.IsMovedError(err); ok { + addr = movedErr.Addr() + addr = internal.GetAddr(addr) + return true, false, addr } - ind := strings.LastIndex(s, " ") - if ind == -1 { - return false, false, "" + // Check for typed AskError + if askErr, ok := proto.IsAskError(err); ok { + addr = askErr.Addr() + addr = internal.GetAddr(addr) + return false, true, addr } - addr = s[ind+1:] - addr = internal.GetAddr(addr) - return + return false, false, "" } func isLoadingError(err error) bool { - return strings.HasPrefix(err.Error(), "LOADING ") + return proto.IsLoadingError(err) } func isReadOnlyError(err error) bool { - return strings.HasPrefix(err.Error(), "READONLY ") + return proto.IsReadOnlyError(err) } func isMovedSameConnAddr(err error, addr string) bool { - redisError := err.Error() - if !strings.HasPrefix(redisError, "MOVED ") { - return false + if movedErr, ok := proto.IsMovedError(err); ok { + return strings.HasSuffix(movedErr.Addr(), addr) + } + return false +} + +//------------------------------------------------------------------------------ + +// Typed error checking functions for public use. +// These functions work correctly even when errors are wrapped in hooks. + +// IsLoadingError checks if an error is a Redis LOADING error, even if wrapped. +// LOADING errors occur when Redis is loading the dataset in memory. +func IsLoadingError(err error) bool { + return proto.IsLoadingError(err) +} + +// IsReadOnlyError checks if an error is a Redis READONLY error, even if wrapped. +// READONLY errors occur when trying to write to a read-only replica. +func IsReadOnlyError(err error) bool { + return proto.IsReadOnlyError(err) +} + +// IsClusterDownError checks if an error is a Redis CLUSTERDOWN error, even if wrapped. +// CLUSTERDOWN errors occur when the cluster is down. +func IsClusterDownError(err error) bool { + return proto.IsClusterDownError(err) +} + +// IsTryAgainError checks if an error is a Redis TRYAGAIN error, even if wrapped. +// TRYAGAIN errors occur when a command cannot be processed and should be retried. +func IsTryAgainError(err error) bool { + return proto.IsTryAgainError(err) +} + +// IsMasterDownError checks if an error is a Redis MASTERDOWN error, even if wrapped. +// MASTERDOWN errors occur when the master is down. +func IsMasterDownError(err error) bool { + return proto.IsMasterDownError(err) +} + +// IsMaxClientsError checks if an error is a Redis max clients error, even if wrapped. +// This error occurs when the maximum number of clients has been reached. +func IsMaxClientsError(err error) bool { + return proto.IsMaxClientsError(err) +} + +// IsMovedError checks if an error is a Redis MOVED error, even if wrapped. +// MOVED errors occur in cluster mode when a key has been moved to a different node. +// Returns the address of the node where the key has been moved and a boolean indicating if it's a MOVED error. +func IsMovedError(err error) (addr string, ok bool) { + if movedErr, isMovedErr := proto.IsMovedError(err); isMovedErr { + return movedErr.Addr(), true + } + return "", false +} + +// IsAskError checks if an error is a Redis ASK error, even if wrapped. +// ASK errors occur in cluster mode when a key is being migrated and the client should ask another node. +// Returns the address of the node to ask and a boolean indicating if it's an ASK error. +func IsAskError(err error) (addr string, ok bool) { + if askErr, isAskErr := proto.IsAskError(err); isAskErr { + return askErr.Addr(), true } - return strings.HasSuffix(redisError, " "+addr) + return "", false } //------------------------------------------------------------------------------ diff --git a/error_wrapping_test.go b/error_wrapping_test.go new file mode 100644 index 0000000000..2ccc9cf8a9 --- /dev/null +++ b/error_wrapping_test.go @@ -0,0 +1,311 @@ +package redis_test + +import ( + "errors" + "fmt" + "testing" + + "github.com/redis/go-redis/v9" + "github.com/redis/go-redis/v9/internal/proto" +) + +// TestTypedErrorsWithHookWrapping demonstrates that typed errors work correctly +// even when wrapped by hooks, which is the main improvement of this change. +func TestTypedErrorsWithHookWrapping(t *testing.T) { + tests := []struct { + name string + errorMsg string + checkFunc func(error) bool + testName string + }{ + { + name: "LOADING error wrapped in hook", + errorMsg: "LOADING Redis is loading the dataset in memory", + checkFunc: redis.IsLoadingError, + testName: "IsLoadingError", + }, + { + name: "READONLY error wrapped in hook", + errorMsg: "READONLY You can't write against a read only replica", + checkFunc: redis.IsReadOnlyError, + testName: "IsReadOnlyError", + }, + { + name: "CLUSTERDOWN error wrapped in hook", + errorMsg: "CLUSTERDOWN The cluster is down", + checkFunc: redis.IsClusterDownError, + testName: "IsClusterDownError", + }, + { + name: "TRYAGAIN error wrapped in hook", + errorMsg: "TRYAGAIN Multiple keys request during rehashing of slot", + checkFunc: redis.IsTryAgainError, + testName: "IsTryAgainError", + }, + { + name: "MASTERDOWN error wrapped in hook", + errorMsg: "MASTERDOWN Link with MASTER is down", + checkFunc: redis.IsMasterDownError, + testName: "IsMasterDownError", + }, + { + name: "Max clients error wrapped in hook", + errorMsg: "ERR max number of clients reached", + checkFunc: redis.IsMaxClientsError, + testName: "IsMaxClientsError", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Simulate a Redis error being created + parsedErr := proto.ParseErrorReply([]byte("-" + tt.errorMsg)) + + // Simulate hook wrapping the error + wrappedErr := fmt.Errorf("hook wrapper: %w", parsedErr) + doubleWrappedErr := fmt.Errorf("another hook: %w", wrappedErr) + + // Test that the typed error check works with wrapped errors + if !tt.checkFunc(doubleWrappedErr) { + t.Errorf("%s failed to detect wrapped error: %v", tt.testName, doubleWrappedErr) + } + + // Test that the error message is still accessible + if !errors.Is(doubleWrappedErr, parsedErr) { + t.Errorf("errors.Is failed to match wrapped error") + } + + // Test that the original error message is preserved in the chain + expectedMsg := tt.errorMsg + if parsedErr.Error() != expectedMsg { + t.Errorf("Error message changed: got %q, want %q", parsedErr.Error(), expectedMsg) + } + + // Verify the generic RedisError interface still works + var redisError redis.Error + if !errors.As(doubleWrappedErr, &redisError) { + t.Errorf("Failed to extract redis.Error from wrapped error") + } + }) + } +} + +// TestMovedAndAskErrorsWithHookWrapping tests MOVED and ASK errors with wrapping +func TestMovedAndAskErrorsWithHookWrapping(t *testing.T) { + tests := []struct { + name string + errorMsg string + expectedAddr string + isMoved bool + }{ + { + name: "MOVED error", + errorMsg: "MOVED 3999 127.0.0.1:6381", + expectedAddr: "127.0.0.1:6381", + isMoved: true, + }, + { + name: "ASK error", + errorMsg: "ASK 3999 192.168.1.100:6380", + expectedAddr: "192.168.1.100:6380", + isMoved: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create the error + parsedErr := proto.ParseErrorReply([]byte("-" + tt.errorMsg)) + + // Wrap it in hooks + wrappedErr := fmt.Errorf("hook wrapper: %w", parsedErr) + doubleWrappedErr := fmt.Errorf("another hook: %w", wrappedErr) + + // Test address extraction from wrapped error + if tt.isMoved { + addr, ok := redis.IsMovedError(doubleWrappedErr) + if !ok { + t.Errorf("IsMovedError failed to detect wrapped MOVED error") + } + if addr != tt.expectedAddr { + t.Errorf("Address mismatch: got %q, want %q", addr, tt.expectedAddr) + } + } else { + addr, ok := redis.IsAskError(doubleWrappedErr) + if !ok { + t.Errorf("IsAskError failed to detect wrapped ASK error") + } + if addr != tt.expectedAddr { + t.Errorf("Address mismatch: got %q, want %q", addr, tt.expectedAddr) + } + } + }) + } +} + +// TestBackwardCompatibilityWithStringChecks verifies that old string-based +// error checking still works for backward compatibility +func TestBackwardCompatibilityWithStringChecks(t *testing.T) { + tests := []struct { + name string + errorMsg string + stringPrefix string + }{ + { + name: "LOADING error", + errorMsg: "LOADING Redis is loading the dataset in memory", + stringPrefix: "LOADING ", + }, + { + name: "READONLY error", + errorMsg: "READONLY You can't write against a read only replica", + stringPrefix: "READONLY ", + }, + { + name: "CLUSTERDOWN error", + errorMsg: "CLUSTERDOWN The cluster is down", + stringPrefix: "CLUSTERDOWN ", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + parsedErr := proto.ParseErrorReply([]byte("-" + tt.errorMsg)) + + // Old-style string checking should still work + errMsg := parsedErr.Error() + if errMsg != tt.errorMsg { + t.Errorf("Error message mismatch: got %q, want %q", errMsg, tt.errorMsg) + } + + // String prefix checking should still work + if len(errMsg) < len(tt.stringPrefix) || errMsg[:len(tt.stringPrefix)] != tt.stringPrefix { + t.Errorf("String prefix check failed: error %q doesn't start with %q", errMsg, tt.stringPrefix) + } + }) + } +} + +// TestErrorWrappingInHookScenario simulates a real-world scenario where +// a hook wraps errors for logging or instrumentation +func TestErrorWrappingInHookScenario(t *testing.T) { + // Simulate a hook that wraps errors for logging + wrapErrorForLogging := func(err error) error { + if err != nil { + return fmt.Errorf("logged error at %s: %w", "2024-01-01T00:00:00Z", err) + } + return nil + } + + // Simulate a hook that adds context + addContextToError := func(err error, cmd string) error { + if err != nil { + return fmt.Errorf("command %s failed: %w", cmd, err) + } + return nil + } + + // Create a LOADING error + loadingErr := proto.ParseErrorReply([]byte("-LOADING Redis is loading the dataset in memory")) + + // Wrap it through multiple hooks + err := loadingErr + err = wrapErrorForLogging(err) + err = addContextToError(err, "GET mykey") + + // The typed error check should still work + if !redis.IsLoadingError(err) { + t.Errorf("IsLoadingError failed to detect error through multiple hook wrappers") + } + + // The error message should contain all the context + errMsg := err.Error() + expectedSubstrings := []string{ + "command GET mykey failed", + "logged error at", + "LOADING Redis is loading the dataset in memory", + } + + for _, substr := range expectedSubstrings { + if !contains(errMsg, substr) { + t.Errorf("Error message missing expected substring %q: %s", substr, errMsg) + } + } +} + +// TestShouldRetryWithTypedErrors tests that shouldRetry works with typed errors +func TestShouldRetryWithTypedErrors(t *testing.T) { + tests := []struct { + name string + errorMsg string + shouldRetry bool + retryTimeout bool + }{ + { + name: "LOADING error should retry", + errorMsg: "LOADING Redis is loading the dataset in memory", + shouldRetry: true, + retryTimeout: false, + }, + { + name: "READONLY error should retry", + errorMsg: "READONLY You can't write against a read only replica", + shouldRetry: true, + retryTimeout: false, + }, + { + name: "CLUSTERDOWN error should retry", + errorMsg: "CLUSTERDOWN The cluster is down", + shouldRetry: true, + retryTimeout: false, + }, + { + name: "TRYAGAIN error should retry", + errorMsg: "TRYAGAIN Multiple keys request during rehashing of slot", + shouldRetry: true, + retryTimeout: false, + }, + { + name: "MASTERDOWN error should retry", + errorMsg: "MASTERDOWN Link with MASTER is down", + shouldRetry: true, + retryTimeout: false, + }, + { + name: "Max clients error should retry", + errorMsg: "ERR max number of clients reached", + shouldRetry: true, + retryTimeout: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := proto.ParseErrorReply([]byte("-" + tt.errorMsg)) + + // Wrap the error + wrappedErr := fmt.Errorf("hook wrapper: %w", err) + + // Test shouldRetry (using the exported ShouldRetry for testing) + result := redis.ShouldRetry(wrappedErr, tt.retryTimeout) + if result != tt.shouldRetry { + t.Errorf("ShouldRetry returned %v, want %v for error: %v", result, tt.shouldRetry, wrappedErr) + } + }) + } +} + +// Helper function to check if a string contains a substring +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && findSubstring(s, substr)) +} + +func findSubstring(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} + diff --git a/internal/proto/reader.go b/internal/proto/reader.go index 4e60569d25..bac68f7965 100644 --- a/internal/proto/reader.go +++ b/internal/proto/reader.go @@ -50,7 +50,8 @@ func (e RedisError) Error() string { return string(e) } func (RedisError) RedisError() {} func ParseErrorReply(line []byte) error { - return RedisError(line[1:]) + msg := string(line[1:]) + return parseTypedRedisError(msg) } //------------------------------------------------------------------------------ @@ -201,7 +202,7 @@ func (r *Reader) ReadLine() ([]byte, error) { var blobErr string blobErr, err = r.readStringReply(line) if err == nil { - err = RedisError(blobErr) + err = parseTypedRedisError(blobErr) } return nil, err case RespAttr: diff --git a/internal/proto/redis_errors.go b/internal/proto/redis_errors.go new file mode 100644 index 0000000000..88a06f732d --- /dev/null +++ b/internal/proto/redis_errors.go @@ -0,0 +1,287 @@ +package proto + +import ( + "errors" + "strings" +) + +// Typed Redis errors for better error handling with wrapping support. +// These errors maintain backward compatibility by keeping the same error messages. + +// LoadingError is returned when Redis is loading the dataset in memory. +type LoadingError struct { + msg string +} + +func (e *LoadingError) Error() string { + return e.msg +} + +func (e *LoadingError) RedisError() {} + +func (e *LoadingError) Is(target error) bool { + _, ok := target.(*LoadingError) + return ok +} + +// NewLoadingError creates a new LoadingError with the given message. +func NewLoadingError(msg string) *LoadingError { + return &LoadingError{msg: msg} +} + +// ReadOnlyError is returned when trying to write to a read-only replica. +type ReadOnlyError struct { + msg string +} + +func (e *ReadOnlyError) Error() string { + return e.msg +} + +func (e *ReadOnlyError) RedisError() {} + +func (e *ReadOnlyError) Is(target error) bool { + _, ok := target.(*ReadOnlyError) + return ok +} + +// NewReadOnlyError creates a new ReadOnlyError with the given message. +func NewReadOnlyError(msg string) *ReadOnlyError { + return &ReadOnlyError{msg: msg} +} + +// MovedError is returned when a key has been moved to a different node in a cluster. +type MovedError struct { + msg string + addr string +} + +func (e *MovedError) Error() string { + return e.msg +} + +func (e *MovedError) RedisError() {} + +func (e *MovedError) Is(target error) bool { + _, ok := target.(*MovedError) + return ok +} + +// Addr returns the address of the node where the key has been moved. +func (e *MovedError) Addr() string { + return e.addr +} + +// NewMovedError creates a new MovedError with the given message and address. +func NewMovedError(msg string, addr string) *MovedError { + return &MovedError{msg: msg, addr: addr} +} + +// AskError is returned when a key is being migrated and the client should ask another node. +type AskError struct { + msg string + addr string +} + +func (e *AskError) Error() string { + return e.msg +} + +func (e *AskError) RedisError() {} + +func (e *AskError) Is(target error) bool { + _, ok := target.(*AskError) + return ok +} + +// Addr returns the address of the node to ask. +func (e *AskError) Addr() string { + return e.addr +} + +// NewAskError creates a new AskError with the given message and address. +func NewAskError(msg string, addr string) *AskError { + return &AskError{msg: msg, addr: addr} +} + +// ClusterDownError is returned when the cluster is down. +type ClusterDownError struct { + msg string +} + +func (e *ClusterDownError) Error() string { + return e.msg +} + +func (e *ClusterDownError) RedisError() {} + +func (e *ClusterDownError) Is(target error) bool { + _, ok := target.(*ClusterDownError) + return ok +} + +// NewClusterDownError creates a new ClusterDownError with the given message. +func NewClusterDownError(msg string) *ClusterDownError { + return &ClusterDownError{msg: msg} +} + +// TryAgainError is returned when a command cannot be processed and should be retried. +type TryAgainError struct { + msg string +} + +func (e *TryAgainError) Error() string { + return e.msg +} + +func (e *TryAgainError) RedisError() {} + +func (e *TryAgainError) Is(target error) bool { + _, ok := target.(*TryAgainError) + return ok +} + +// NewTryAgainError creates a new TryAgainError with the given message. +func NewTryAgainError(msg string) *TryAgainError { + return &TryAgainError{msg: msg} +} + +// MasterDownError is returned when the master is down. +type MasterDownError struct { + msg string +} + +func (e *MasterDownError) Error() string { + return e.msg +} + +func (e *MasterDownError) RedisError() {} + +func (e *MasterDownError) Is(target error) bool { + _, ok := target.(*MasterDownError) + return ok +} + +// NewMasterDownError creates a new MasterDownError with the given message. +func NewMasterDownError(msg string) *MasterDownError { + return &MasterDownError{msg: msg} +} + +// MaxClientsError is returned when the maximum number of clients has been reached. +type MaxClientsError struct { + msg string +} + +func (e *MaxClientsError) Error() string { + return e.msg +} + +func (e *MaxClientsError) RedisError() {} + +func (e *MaxClientsError) Is(target error) bool { + _, ok := target.(*MaxClientsError) + return ok +} + +// NewMaxClientsError creates a new MaxClientsError with the given message. +func NewMaxClientsError(msg string) *MaxClientsError { + return &MaxClientsError{msg: msg} +} + +// parseTypedRedisError parses a Redis error message and returns a typed error if applicable. +// This function maintains backward compatibility by keeping the same error messages. +func parseTypedRedisError(msg string) error { + // Check for specific error patterns and return typed errors + switch { + case strings.HasPrefix(msg, "LOADING "): + return NewLoadingError(msg) + case strings.HasPrefix(msg, "READONLY "): + return NewReadOnlyError(msg) + case strings.HasPrefix(msg, "MOVED "): + // Extract address from "MOVED " + addr := extractAddr(msg) + return NewMovedError(msg, addr) + case strings.HasPrefix(msg, "ASK "): + // Extract address from "ASK " + addr := extractAddr(msg) + return NewAskError(msg, addr) + case strings.HasPrefix(msg, "CLUSTERDOWN "): + return NewClusterDownError(msg) + case strings.HasPrefix(msg, "TRYAGAIN "): + return NewTryAgainError(msg) + case strings.HasPrefix(msg, "MASTERDOWN "): + return NewMasterDownError(msg) + case msg == "ERR max number of clients reached": + return NewMaxClientsError(msg) + default: + // Return generic RedisError for unknown error types + return RedisError(msg) + } +} + +// extractAddr extracts the address from MOVED/ASK error messages. +// Format: "MOVED " or "ASK " +func extractAddr(msg string) string { + ind := strings.LastIndex(msg, " ") + if ind == -1 { + return "" + } + return msg[ind+1:] +} + +// IsLoadingError checks if an error is a LoadingError, even if wrapped. +func IsLoadingError(err error) bool { + var loadingErr *LoadingError + return errors.As(err, &loadingErr) +} + +// IsReadOnlyError checks if an error is a ReadOnlyError, even if wrapped. +func IsReadOnlyError(err error) bool { + var readOnlyErr *ReadOnlyError + return errors.As(err, &readOnlyErr) +} + +// IsMovedError checks if an error is a MovedError, even if wrapped. +// Returns the error and a boolean indicating if it's a MovedError. +func IsMovedError(err error) (*MovedError, bool) { + var movedErr *MovedError + if errors.As(err, &movedErr) { + return movedErr, true + } + return nil, false +} + +// IsAskError checks if an error is an AskError, even if wrapped. +// Returns the error and a boolean indicating if it's an AskError. +func IsAskError(err error) (*AskError, bool) { + var askErr *AskError + if errors.As(err, &askErr) { + return askErr, true + } + return nil, false +} + +// IsClusterDownError checks if an error is a ClusterDownError, even if wrapped. +func IsClusterDownError(err error) bool { + var clusterDownErr *ClusterDownError + return errors.As(err, &clusterDownErr) +} + +// IsTryAgainError checks if an error is a TryAgainError, even if wrapped. +func IsTryAgainError(err error) bool { + var tryAgainErr *TryAgainError + return errors.As(err, &tryAgainErr) +} + +// IsMasterDownError checks if an error is a MasterDownError, even if wrapped. +func IsMasterDownError(err error) bool { + var masterDownErr *MasterDownError + return errors.As(err, &masterDownErr) +} + +// IsMaxClientsError checks if an error is a MaxClientsError, even if wrapped. +func IsMaxClientsError(err error) bool { + var maxClientsErr *MaxClientsError + return errors.As(err, &maxClientsErr) +} + diff --git a/internal/proto/redis_errors_test.go b/internal/proto/redis_errors_test.go new file mode 100644 index 0000000000..c6ab081fa7 --- /dev/null +++ b/internal/proto/redis_errors_test.go @@ -0,0 +1,319 @@ +package proto + +import ( + "errors" + "fmt" + "testing" +) + +// TestTypedRedisErrors tests that typed Redis errors are created correctly +func TestTypedRedisErrors(t *testing.T) { + tests := []struct { + name string + errorMsg string + expectedType interface{} + expectedMsg string + checkFunc func(error) bool + extractAddr func(error) string + }{ + { + name: "LOADING error", + errorMsg: "LOADING Redis is loading the dataset in memory", + expectedType: &LoadingError{}, + expectedMsg: "LOADING Redis is loading the dataset in memory", + checkFunc: IsLoadingError, + }, + { + name: "READONLY error", + errorMsg: "READONLY You can't write against a read only replica", + expectedType: &ReadOnlyError{}, + expectedMsg: "READONLY You can't write against a read only replica", + checkFunc: IsReadOnlyError, + }, + { + name: "MOVED error", + errorMsg: "MOVED 3999 127.0.0.1:6381", + expectedType: &MovedError{}, + expectedMsg: "MOVED 3999 127.0.0.1:6381", + checkFunc: func(err error) bool { + _, ok := IsMovedError(err) + return ok + }, + extractAddr: func(err error) string { + if movedErr, ok := IsMovedError(err); ok { + return movedErr.Addr() + } + return "" + }, + }, + { + name: "ASK error", + errorMsg: "ASK 3999 127.0.0.1:6381", + expectedType: &AskError{}, + expectedMsg: "ASK 3999 127.0.0.1:6381", + checkFunc: func(err error) bool { + _, ok := IsAskError(err) + return ok + }, + extractAddr: func(err error) string { + if askErr, ok := IsAskError(err); ok { + return askErr.Addr() + } + return "" + }, + }, + { + name: "CLUSTERDOWN error", + errorMsg: "CLUSTERDOWN The cluster is down", + expectedType: &ClusterDownError{}, + expectedMsg: "CLUSTERDOWN The cluster is down", + checkFunc: IsClusterDownError, + }, + { + name: "TRYAGAIN error", + errorMsg: "TRYAGAIN Multiple keys request during rehashing of slot", + expectedType: &TryAgainError{}, + expectedMsg: "TRYAGAIN Multiple keys request during rehashing of slot", + checkFunc: IsTryAgainError, + }, + { + name: "MASTERDOWN error", + errorMsg: "MASTERDOWN Link with MASTER is down and replica-serve-stale-data is set to 'no'", + expectedType: &MasterDownError{}, + expectedMsg: "MASTERDOWN Link with MASTER is down and replica-serve-stale-data is set to 'no'", + checkFunc: IsMasterDownError, + }, + { + name: "Max clients error", + errorMsg: "ERR max number of clients reached", + expectedType: &MaxClientsError{}, + expectedMsg: "ERR max number of clients reached", + checkFunc: IsMaxClientsError, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := parseTypedRedisError(tt.errorMsg) + + // Check error message is preserved + if err.Error() != tt.expectedMsg { + t.Errorf("Error message mismatch: got %q, want %q", err.Error(), tt.expectedMsg) + } + + // Check error type using errors.As + if !errors.As(err, &tt.expectedType) { + t.Errorf("Error type mismatch: expected %T, got %T", tt.expectedType, err) + } + + // Check using the helper function + if tt.checkFunc != nil && !tt.checkFunc(err) { + t.Errorf("Helper function returned false for error: %v", err) + } + + // Check address extraction for MOVED/ASK errors + if tt.extractAddr != nil { + addr := tt.extractAddr(err) + if addr == "" { + t.Errorf("Failed to extract address from error: %v", err) + } + } + }) + } +} + +// TestWrappedTypedErrors tests that typed errors work correctly when wrapped +func TestWrappedTypedErrors(t *testing.T) { + tests := []struct { + name string + errorMsg string + checkFunc func(error) bool + }{ + { + name: "Wrapped LOADING error", + errorMsg: "LOADING Redis is loading the dataset in memory", + checkFunc: IsLoadingError, + }, + { + name: "Wrapped READONLY error", + errorMsg: "READONLY You can't write against a read only replica", + checkFunc: IsReadOnlyError, + }, + { + name: "Wrapped CLUSTERDOWN error", + errorMsg: "CLUSTERDOWN The cluster is down", + checkFunc: IsClusterDownError, + }, + { + name: "Wrapped TRYAGAIN error", + errorMsg: "TRYAGAIN Multiple keys request during rehashing of slot", + checkFunc: IsTryAgainError, + }, + { + name: "Wrapped MASTERDOWN error", + errorMsg: "MASTERDOWN Link with MASTER is down", + checkFunc: IsMasterDownError, + }, + { + name: "Wrapped Max clients error", + errorMsg: "ERR max number of clients reached", + checkFunc: IsMaxClientsError, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create the typed error + err := parseTypedRedisError(tt.errorMsg) + + // Wrap it multiple times (simulating hook wrapping) + wrappedErr := fmt.Errorf("hook error: %w", err) + doubleWrappedErr := fmt.Errorf("another wrapper: %w", wrappedErr) + + // Check that the helper function still works with wrapped errors + if !tt.checkFunc(doubleWrappedErr) { + t.Errorf("Helper function failed to detect wrapped error: %v", doubleWrappedErr) + } + + // Verify the original error message is still accessible + if !errors.Is(doubleWrappedErr, err) { + t.Errorf("errors.Is failed to match wrapped error") + } + }) + } +} + +// TestMovedAndAskErrorAddressExtraction tests address extraction from MOVED/ASK errors +func TestMovedAndAskErrorAddressExtraction(t *testing.T) { + tests := []struct { + name string + errorMsg string + expectedAddr string + }{ + { + name: "MOVED with IP address", + errorMsg: "MOVED 3999 127.0.0.1:6381", + expectedAddr: "127.0.0.1:6381", + }, + { + name: "MOVED with hostname", + errorMsg: "MOVED 3999 redis-node-1:6379", + expectedAddr: "redis-node-1:6379", + }, + { + name: "ASK with IP address", + errorMsg: "ASK 3999 192.168.1.100:6380", + expectedAddr: "192.168.1.100:6380", + }, + { + name: "ASK with hostname", + errorMsg: "ASK 3999 redis-node-2:6379", + expectedAddr: "redis-node-2:6379", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := parseTypedRedisError(tt.errorMsg) + + var addr string + if movedErr, ok := IsMovedError(err); ok { + addr = movedErr.Addr() + } else if askErr, ok := IsAskError(err); ok { + addr = askErr.Addr() + } else { + t.Fatalf("Error is neither MOVED nor ASK: %v", err) + } + + if addr != tt.expectedAddr { + t.Errorf("Address mismatch: got %q, want %q", addr, tt.expectedAddr) + } + + // Test with wrapped error + wrappedErr := fmt.Errorf("wrapped: %w", err) + if movedErr, ok := IsMovedError(wrappedErr); ok { + addr = movedErr.Addr() + } else if askErr, ok := IsAskError(wrappedErr); ok { + addr = askErr.Addr() + } else { + t.Fatalf("Wrapped error is neither MOVED nor ASK: %v", wrappedErr) + } + + if addr != tt.expectedAddr { + t.Errorf("Address mismatch in wrapped error: got %q, want %q", addr, tt.expectedAddr) + } + }) + } +} + +// TestGenericRedisError tests that unknown Redis errors fall back to generic RedisError +func TestGenericRedisError(t *testing.T) { + tests := []struct { + name string + errorMsg string + }{ + { + name: "Generic error", + errorMsg: "ERR unknown command", + }, + { + name: "WRONGTYPE error", + errorMsg: "WRONGTYPE Operation against a key holding the wrong kind of value", + }, + { + name: "NOAUTH error", + errorMsg: "NOAUTH Authentication required", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := parseTypedRedisError(tt.errorMsg) + + // Should be a generic RedisError + if _, ok := err.(RedisError); !ok { + t.Errorf("Expected RedisError, got %T", err) + } + + // Should preserve the error message + if err.Error() != tt.errorMsg { + t.Errorf("Error message mismatch: got %q, want %q", err.Error(), tt.errorMsg) + } + + // Should not match any typed error checks + if IsLoadingError(err) || IsReadOnlyError(err) || IsClusterDownError(err) || + IsTryAgainError(err) || IsMasterDownError(err) || IsMaxClientsError(err) { + t.Errorf("Generic error incorrectly matched a typed error check") + } + }) + } +} + +// TestBackwardCompatibility tests that error messages remain unchanged +func TestBackwardCompatibility(t *testing.T) { + // This test ensures that the error messages are exactly the same as before + // to maintain backward compatibility with code that checks error messages + tests := []struct { + input string + expected string + }{ + {"LOADING Redis is loading the dataset in memory", "LOADING Redis is loading the dataset in memory"}, + {"READONLY You can't write against a read only replica", "READONLY You can't write against a read only replica"}, + {"MOVED 3999 127.0.0.1:6381", "MOVED 3999 127.0.0.1:6381"}, + {"ASK 3999 127.0.0.1:6381", "ASK 3999 127.0.0.1:6381"}, + {"CLUSTERDOWN The cluster is down", "CLUSTERDOWN The cluster is down"}, + {"TRYAGAIN Multiple keys request during rehashing of slot", "TRYAGAIN Multiple keys request during rehashing of slot"}, + {"MASTERDOWN Link with MASTER is down", "MASTERDOWN Link with MASTER is down"}, + {"ERR max number of clients reached", "ERR max number of clients reached"}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + err := parseTypedRedisError(tt.input) + if err.Error() != tt.expected { + t.Errorf("Error message changed! Got %q, want %q", err.Error(), tt.expected) + } + }) + } +} + From 3588618c177519f5793cdb23c868244a43840e9e Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Tue, 18 Nov 2025 15:28:51 +0200 Subject: [PATCH 02/12] add error documentation --- README.md | 70 +++++++++++++++++++ error_wrapping_test.go | 148 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+) diff --git a/README.md b/README.md index 071640355b..4debccfe15 100644 --- a/README.md +++ b/README.md @@ -429,6 +429,76 @@ vals, err := rdb.Eval(ctx, "return {KEYS[1],ARGV[1]}", []string{"key"}, "hello") res, err := rdb.Do(ctx, "set", "key", "value").Result() ``` +## Typed Errors + +go-redis provides typed error checking functions for common Redis errors: + +```go +redis.IsLoadingError(err) // Redis is loading the dataset +redis.IsReadOnlyError(err) // Write to read-only replica +redis.IsClusterDownError(err) // Cluster is down +redis.IsTryAgainError(err) // Command should be retried +redis.IsMasterDownError(err) // Master is down +redis.IsMaxClientsError(err) // Maximum clients reached +redis.IsMovedError(err) // Returns (address, true) if key moved +redis.IsAskError(err) // Returns (address, true) if key being migrated +``` + +### Error Wrapping in Hooks + +When wrapping errors in hooks, use custom error types with `Unwrap()` method (preferred) or `fmt.Errorf` with `%w`. Always call `cmd.SetErr()` to preserve error type information: + +```go +// Custom error type (preferred) +type AppError struct { + Code string + RequestID string + Err error +} + +func (e *AppError) Error() string { + return fmt.Sprintf("[%s] request_id=%s: %v", e.Code, e.RequestID, e.Err) +} + +func (e *AppError) Unwrap() error { + return e.Err +} + +// Hook implementation +func (h MyHook) ProcessHook(next redis.ProcessHook) redis.ProcessHook { + return func(ctx context.Context, cmd redis.Cmder) error { + err := next(ctx, cmd) + if err != nil { + // Wrap with custom error type + wrappedErr := &AppError{ + Code: "REDIS_ERROR", + RequestID: getRequestID(ctx), + Err: err, + } + cmd.SetErr(wrappedErr) + return wrappedErr + } + return nil + } +} + +// Typed error detection works through wrappers +if redis.IsLoadingError(err) { + // Retry logic +} + +// Extract custom error if needed +var appErr *AppError +if errors.As(err, &appErr) { + log.Printf("Request: %s", appErr.RequestID) +} +``` + +Alternatively, use `fmt.Errorf` with `%w`: +```go +wrappedErr := fmt.Errorf("context: %w", err) +cmd.SetErr(wrappedErr) +``` ## Run the test diff --git a/error_wrapping_test.go b/error_wrapping_test.go index 2ccc9cf8a9..62ef4f07f9 100644 --- a/error_wrapping_test.go +++ b/error_wrapping_test.go @@ -1,6 +1,7 @@ package redis_test import ( + "context" "errors" "fmt" "testing" @@ -295,6 +296,153 @@ func TestShouldRetryWithTypedErrors(t *testing.T) { } } +// TestSetErrWithWrappedError tests that when a hook wraps an error and sets it +// via cmd.SetErr(), the underlying typed error can still be detected +func TestSetErrWithWrappedError(t *testing.T) { + testCtx := context.Background() + + // Test with a simulated LOADING error + // We test the mechanism directly without needing a real Redis server + cmd := redis.NewStatusCmd(testCtx, "GET", "key") + loadingErr := proto.ParseErrorReply([]byte("-LOADING Redis is loading the dataset in memory")) + wrappedLoadingErr := fmt.Errorf("hook wrapper: %w", loadingErr) + cmd.SetErr(wrappedLoadingErr) + + // Verify we can still detect the LOADING error through the wrapper + if !redis.IsLoadingError(cmd.Err()) { + t.Errorf("IsLoadingError failed to detect wrapped error set via SetErr: %v", cmd.Err()) + } + + // Test with MOVED error + cmd2 := redis.NewStatusCmd(testCtx, "GET", "key") + movedErr := proto.ParseErrorReply([]byte("-MOVED 3999 127.0.0.1:6381")) + wrappedMovedErr := fmt.Errorf("hook wrapper: %w", movedErr) + cmd2.SetErr(wrappedMovedErr) + + // Verify we can still detect and extract address from MOVED error + addr, ok := redis.IsMovedError(cmd2.Err()) + if !ok { + t.Errorf("IsMovedError failed to detect wrapped error set via SetErr: %v", cmd2.Err()) + } + if addr != "127.0.0.1:6381" { + t.Errorf("Address extraction failed: got %q, want %q", addr, "127.0.0.1:6381") + } + + // Test with READONLY error + cmd3 := redis.NewStatusCmd(testCtx, "SET", "key", "value") + readonlyErr := proto.ParseErrorReply([]byte("-READONLY You can't write against a read only replica")) + wrappedReadonlyErr := fmt.Errorf("custom error wrapper: %w", readonlyErr) + cmd3.SetErr(wrappedReadonlyErr) + + // Verify we can still detect the READONLY error through the wrapper + if !redis.IsReadOnlyError(cmd3.Err()) { + t.Errorf("IsReadOnlyError failed to detect wrapped error set via SetErr: %v", cmd3.Err()) + } + + // Verify the error message contains both the wrapper and original error + errMsg := cmd3.Err().Error() + if !contains(errMsg, "custom error wrapper") { + t.Errorf("Error message missing wrapper context: %v", errMsg) + } + if !contains(errMsg, "READONLY") { + t.Errorf("Error message missing original error: %v", errMsg) + } +} + +// AppError is a custom error type for testing +type AppError struct { + Code string + Message string + RequestID string + Err error +} + +// Error implements the error interface +func (e *AppError) Error() string { + return fmt.Sprintf("[%s] %s (request_id=%s): %v", e.Code, e.Message, e.RequestID, e.Err) +} + +// Unwrap implements the error unwrapping interface - this is critical for errors.As() to work +func (e *AppError) Unwrap() error { + return e.Err +} + +// TestCustomErrorTypeWrapping tests that users can wrap Redis errors in their own custom error types +// and still have typed error detection work correctly +func TestCustomErrorTypeWrapping(t *testing.T) { + testCtx := context.Background() + + // Test 1: Wrap LOADING error in custom type + cmd1 := redis.NewStatusCmd(testCtx, "GET", "key") + loadingErr := proto.ParseErrorReply([]byte("-LOADING Redis is loading the dataset in memory")) + customErr1 := &AppError{ + Code: "REDIS_ERROR", + Message: "Database operation failed", + RequestID: "req-12345", + Err: loadingErr, + } + cmd1.SetErr(customErr1) + + // Verify typed error detection works through custom error type + if !redis.IsLoadingError(cmd1.Err()) { + t.Errorf("IsLoadingError failed to detect error wrapped in custom type: %v", cmd1.Err()) + } + + // Verify error message contains custom context + errMsg := cmd1.Err().Error() + if !contains(errMsg, "REDIS_ERROR") || !contains(errMsg, "req-12345") { + t.Errorf("Error message missing custom error context: %v", errMsg) + } + + // Test 2: Wrap MOVED error in custom type + cmd2 := redis.NewStatusCmd(testCtx, "GET", "key") + movedErr := proto.ParseErrorReply([]byte("-MOVED 3999 127.0.0.1:6381")) + customErr2 := &AppError{ + Code: "CLUSTER_REDIRECT", + Message: "Key moved to different node", + RequestID: "req-67890", + Err: movedErr, + } + cmd2.SetErr(customErr2) + + // Verify address extraction works through custom error type + addr, ok := redis.IsMovedError(cmd2.Err()) + if !ok { + t.Errorf("IsMovedError failed to detect error wrapped in custom type: %v", cmd2.Err()) + } + if addr != "127.0.0.1:6381" { + t.Errorf("Address extraction failed: got %q, want %q", addr, "127.0.0.1:6381") + } + + // Test 3: Multiple levels of wrapping (custom type + fmt.Errorf) + cmd3 := redis.NewStatusCmd(testCtx, "SET", "key", "value") + readonlyErr := proto.ParseErrorReply([]byte("-READONLY You can't write against a read only replica")) + customErr3 := &AppError{ + Code: "WRITE_ERROR", + Message: "Write operation failed", + RequestID: "req-11111", + Err: readonlyErr, + } + // Wrap the custom error again with fmt.Errorf + doubleWrapped := fmt.Errorf("hook context: %w", customErr3) + cmd3.SetErr(doubleWrapped) + + // Verify typed error detection works through multiple levels of wrapping + if !redis.IsReadOnlyError(cmd3.Err()) { + t.Errorf("IsReadOnlyError failed to detect error wrapped in custom type + fmt.Errorf: %v", cmd3.Err()) + } + + // Verify we can unwrap to get the custom error + var appErr *AppError + if !errors.As(cmd3.Err(), &appErr) { + t.Errorf("errors.As failed to extract custom error type from wrapped error") + } else { + if appErr.Code != "WRITE_ERROR" || appErr.RequestID != "req-11111" { + t.Errorf("Custom error fields incorrect: Code=%s, RequestID=%s", appErr.Code, appErr.RequestID) + } + } +} + // Helper function to check if a string contains a substring func contains(s, substr string) bool { return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && findSubstring(s, substr)) From ba34dadf3407cbd9d878321453c1afe9f503a923 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Tue, 18 Nov 2025 15:49:01 +0200 Subject: [PATCH 03/12] backwards compatibility --- error.go | 41 ++++++++++++++++-- error_test.go | 15 ++++--- internal/proto/redis_errors.go | 78 +++++++++++++++++++++++++++++++--- 3 files changed, 117 insertions(+), 17 deletions(-) diff --git a/error.go b/error.go index 33c0651200..7954db5472 100644 --- a/error.go +++ b/error.go @@ -98,6 +98,24 @@ func shouldRetry(err error, retryTimeout bool) bool { return true } + // Fallback to string checking for backward compatibility with plain errors + s := err.Error() + if strings.HasPrefix(s, "ERR max number of clients reached") { + return true + } + if strings.HasPrefix(s, "LOADING ") { + return true + } + if strings.HasPrefix(s, "READONLY ") { + return true + } + if strings.HasPrefix(s, "CLUSTERDOWN ") { + return true + } + if strings.HasPrefix(s, "TRYAGAIN ") { + return true + } + return false } @@ -142,10 +160,6 @@ func isBadConn(err error, allowTimeout bool, addr string) bool { } func isMovedError(err error) (moved bool, ask bool, addr string) { - if !isRedisError(err) { - return - } - // Check for typed MovedError if movedErr, ok := proto.IsMovedError(err); ok { addr = movedErr.Addr() @@ -160,6 +174,25 @@ func isMovedError(err error) (moved bool, ask bool, addr string) { return false, true, addr } + // Fallback to string checking for backward compatibility + s := err.Error() + if strings.HasPrefix(s, "MOVED ") { + // Parse: MOVED 3999 127.0.0.1:6381 + parts := strings.Split(s, " ") + if len(parts) == 3 { + addr = internal.GetAddr(parts[2]) + return true, false, addr + } + } + if strings.HasPrefix(s, "ASK ") { + // Parse: ASK 3999 127.0.0.1:6381 + parts := strings.Split(s, " ") + if len(parts) == 3 { + addr = internal.GetAddr(parts[2]) + return false, true, addr + } + } + return false, false, "" } diff --git a/error_test.go b/error_test.go index da9a471a28..6c130d38ae 100644 --- a/error_test.go +++ b/error_test.go @@ -2,12 +2,12 @@ package redis_test import ( "context" - "errors" "io" . "github.com/bsm/ginkgo/v2" . "github.com/bsm/gomega" "github.com/redis/go-redis/v9" + "github.com/redis/go-redis/v9/internal/proto" ) type testTimeout struct { @@ -39,12 +39,13 @@ var _ = Describe("error", func() { context.Canceled: false, context.DeadlineExceeded: false, redis.ErrPoolTimeout: true, - errors.New("ERR max number of clients reached"): true, - errors.New("LOADING Redis is loading the dataset in memory"): true, - errors.New("READONLY You can't write against a read only replica"): true, - errors.New("CLUSTERDOWN The cluster is down"): true, - errors.New("TRYAGAIN Command cannot be processed, please try again"): true, - errors.New("other"): false, + // Use typed errors instead of plain errors.New() + proto.ParseErrorReply([]byte("-ERR max number of clients reached")): true, + proto.ParseErrorReply([]byte("-LOADING Redis is loading the dataset in memory")): true, + proto.ParseErrorReply([]byte("-READONLY You can't write against a read only replica")): true, + proto.ParseErrorReply([]byte("-CLUSTERDOWN The cluster is down")): true, + proto.ParseErrorReply([]byte("-TRYAGAIN Command cannot be processed, please try again")): true, + proto.ParseErrorReply([]byte("-ERR other")): false, } for err, expected := range data { diff --git a/internal/proto/redis_errors.go b/internal/proto/redis_errors.go index 88a06f732d..5ef45248a0 100644 --- a/internal/proto/redis_errors.go +++ b/internal/proto/redis_errors.go @@ -231,57 +231,123 @@ func extractAddr(msg string) string { // IsLoadingError checks if an error is a LoadingError, even if wrapped. func IsLoadingError(err error) bool { + if err == nil { + return false + } var loadingErr *LoadingError - return errors.As(err, &loadingErr) + if errors.As(err, &loadingErr) { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "LOADING ") } // IsReadOnlyError checks if an error is a ReadOnlyError, even if wrapped. func IsReadOnlyError(err error) bool { + if err == nil { + return false + } var readOnlyErr *ReadOnlyError - return errors.As(err, &readOnlyErr) + if errors.As(err, &readOnlyErr) { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "READONLY ") } // IsMovedError checks if an error is a MovedError, even if wrapped. // Returns the error and a boolean indicating if it's a MovedError. func IsMovedError(err error) (*MovedError, bool) { + if err == nil { + return nil, false + } var movedErr *MovedError if errors.As(err, &movedErr) { return movedErr, true } + // Fallback to string checking for backward compatibility + s := err.Error() + if strings.HasPrefix(s, "MOVED ") { + // Parse: MOVED 3999 127.0.0.1:6381 + parts := strings.Split(s, " ") + if len(parts) == 3 { + return &MovedError{msg: s}, true + } + } return nil, false } // IsAskError checks if an error is an AskError, even if wrapped. // Returns the error and a boolean indicating if it's an AskError. func IsAskError(err error) (*AskError, bool) { + if err == nil { + return nil, false + } var askErr *AskError if errors.As(err, &askErr) { return askErr, true } + // Fallback to string checking for backward compatibility + s := err.Error() + if strings.HasPrefix(s, "ASK ") { + // Parse: ASK 3999 127.0.0.1:6381 + parts := strings.Split(s, " ") + if len(parts) == 3 { + return &AskError{msg: s}, true + } + } return nil, false } // IsClusterDownError checks if an error is a ClusterDownError, even if wrapped. func IsClusterDownError(err error) bool { + if err == nil { + return false + } var clusterDownErr *ClusterDownError - return errors.As(err, &clusterDownErr) + if errors.As(err, &clusterDownErr) { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "CLUSTERDOWN ") } // IsTryAgainError checks if an error is a TryAgainError, even if wrapped. func IsTryAgainError(err error) bool { + if err == nil { + return false + } var tryAgainErr *TryAgainError - return errors.As(err, &tryAgainErr) + if errors.As(err, &tryAgainErr) { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "TRYAGAIN ") } // IsMasterDownError checks if an error is a MasterDownError, even if wrapped. func IsMasterDownError(err error) bool { + if err == nil { + return false + } var masterDownErr *MasterDownError - return errors.As(err, &masterDownErr) + if errors.As(err, &masterDownErr) { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "MASTERDOWN ") } // IsMaxClientsError checks if an error is a MaxClientsError, even if wrapped. func IsMaxClientsError(err error) bool { + if err == nil { + return false + } var maxClientsErr *MaxClientsError - return errors.As(err, &maxClientsErr) + if errors.As(err, &maxClientsErr) { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "ERR max number of clients reached") } From 631073294e59d9e22ecf6b3c30af93bcbe4ae5d4 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Tue, 18 Nov 2025 16:32:55 +0200 Subject: [PATCH 04/12] update readme, remove Is methods --- README.md | 2 +- internal/proto/redis_errors.go | 40 ---------------------------------- 2 files changed, 1 insertion(+), 41 deletions(-) diff --git a/README.md b/README.md index 4debccfe15..dc3808d81b 100644 --- a/README.md +++ b/README.md @@ -476,7 +476,7 @@ func (h MyHook) ProcessHook(next redis.ProcessHook) redis.ProcessHook { Err: err, } cmd.SetErr(wrappedErr) - return wrappedErr + return wrappedErr // Return wrapped error to preserve it } return nil } diff --git a/internal/proto/redis_errors.go b/internal/proto/redis_errors.go index 5ef45248a0..80e76eeb4e 100644 --- a/internal/proto/redis_errors.go +++ b/internal/proto/redis_errors.go @@ -19,11 +19,6 @@ func (e *LoadingError) Error() string { func (e *LoadingError) RedisError() {} -func (e *LoadingError) Is(target error) bool { - _, ok := target.(*LoadingError) - return ok -} - // NewLoadingError creates a new LoadingError with the given message. func NewLoadingError(msg string) *LoadingError { return &LoadingError{msg: msg} @@ -40,11 +35,6 @@ func (e *ReadOnlyError) Error() string { func (e *ReadOnlyError) RedisError() {} -func (e *ReadOnlyError) Is(target error) bool { - _, ok := target.(*ReadOnlyError) - return ok -} - // NewReadOnlyError creates a new ReadOnlyError with the given message. func NewReadOnlyError(msg string) *ReadOnlyError { return &ReadOnlyError{msg: msg} @@ -62,11 +52,6 @@ func (e *MovedError) Error() string { func (e *MovedError) RedisError() {} -func (e *MovedError) Is(target error) bool { - _, ok := target.(*MovedError) - return ok -} - // Addr returns the address of the node where the key has been moved. func (e *MovedError) Addr() string { return e.addr @@ -89,11 +74,6 @@ func (e *AskError) Error() string { func (e *AskError) RedisError() {} -func (e *AskError) Is(target error) bool { - _, ok := target.(*AskError) - return ok -} - // Addr returns the address of the node to ask. func (e *AskError) Addr() string { return e.addr @@ -115,11 +95,6 @@ func (e *ClusterDownError) Error() string { func (e *ClusterDownError) RedisError() {} -func (e *ClusterDownError) Is(target error) bool { - _, ok := target.(*ClusterDownError) - return ok -} - // NewClusterDownError creates a new ClusterDownError with the given message. func NewClusterDownError(msg string) *ClusterDownError { return &ClusterDownError{msg: msg} @@ -136,11 +111,6 @@ func (e *TryAgainError) Error() string { func (e *TryAgainError) RedisError() {} -func (e *TryAgainError) Is(target error) bool { - _, ok := target.(*TryAgainError) - return ok -} - // NewTryAgainError creates a new TryAgainError with the given message. func NewTryAgainError(msg string) *TryAgainError { return &TryAgainError{msg: msg} @@ -157,11 +127,6 @@ func (e *MasterDownError) Error() string { func (e *MasterDownError) RedisError() {} -func (e *MasterDownError) Is(target error) bool { - _, ok := target.(*MasterDownError) - return ok -} - // NewMasterDownError creates a new MasterDownError with the given message. func NewMasterDownError(msg string) *MasterDownError { return &MasterDownError{msg: msg} @@ -178,11 +143,6 @@ func (e *MaxClientsError) Error() string { func (e *MaxClientsError) RedisError() {} -func (e *MaxClientsError) Is(target error) bool { - _, ok := target.(*MaxClientsError) - return ok -} - // NewMaxClientsError creates a new MaxClientsError with the given message. func NewMaxClientsError(msg string) *MaxClientsError { return &MaxClientsError{msg: msg} From bdfd0e91a1e9a3966389f4279bb4bf64c1b9b505 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> Date: Tue, 18 Nov 2025 17:10:16 +0200 Subject: [PATCH 05/12] Update internal/proto/redis_errors.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/proto/redis_errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/proto/redis_errors.go b/internal/proto/redis_errors.go index 80e76eeb4e..374f42d235 100644 --- a/internal/proto/redis_errors.go +++ b/internal/proto/redis_errors.go @@ -231,7 +231,7 @@ func IsMovedError(err error) (*MovedError, bool) { // Parse: MOVED 3999 127.0.0.1:6381 parts := strings.Split(s, " ") if len(parts) == 3 { - return &MovedError{msg: s}, true + return &MovedError{msg: s, addr: parts[2]}, true } } return nil, false From b69cdb651ea31d364a1e7972a0587ada921af723 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> Date: Tue, 18 Nov 2025 17:10:24 +0200 Subject: [PATCH 06/12] Update internal/proto/redis_errors.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/proto/redis_errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/proto/redis_errors.go b/internal/proto/redis_errors.go index 374f42d235..5fdf133c19 100644 --- a/internal/proto/redis_errors.go +++ b/internal/proto/redis_errors.go @@ -253,7 +253,7 @@ func IsAskError(err error) (*AskError, bool) { // Parse: ASK 3999 127.0.0.1:6381 parts := strings.Split(s, " ") if len(parts) == 3 { - return &AskError{msg: s}, true + return &AskError{msg: s, addr: parts[2]}, true } } return nil, false From ce1a7c466988c63d2de8a62473b6803572a39964 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Wed, 19 Nov 2025 14:52:57 +0200 Subject: [PATCH 07/12] support error wrapping for io and context errors --- error.go | 85 ++++++++++---- error_wrapping_test.go | 201 +++++++++++++++++++++++++++++++-- internal/proto/redis_errors.go | 30 +++++ 3 files changed, 286 insertions(+), 30 deletions(-) diff --git a/error.go b/error.go index 7954db5472..033b235bdd 100644 --- a/error.go +++ b/error.go @@ -52,27 +52,54 @@ type Error interface { var _ Error = proto.RedisError("") func isContextError(err error) bool { - switch err { - case context.Canceled, context.DeadlineExceeded: - return true - default: - return false + // Check for wrapped context errors using errors.Is + return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) +} + +// isTimeoutError checks if an error is a timeout error, even if wrapped. +// Returns (isTimeout, shouldRetryOnTimeout) where: +// - isTimeout: true if the error is any kind of timeout error +// - shouldRetryOnTimeout: true if Timeout() method returns true +func isTimeoutError(err error) (isTimeout bool, hasTimeoutFlag bool) { + // Check for timeoutError interface (works with wrapped errors) + var te timeoutError + if errors.As(err, &te) { + return true, te.Timeout() } + + // Check for net.Error specifically (common case for network timeouts) + var netErr net.Error + if errors.As(err, &netErr) { + return true, netErr.Timeout() + } + + return false, false } func shouldRetry(err error, retryTimeout bool) bool { - switch err { - case io.EOF, io.ErrUnexpectedEOF: + if err == nil { + return false + } + + // Check for EOF errors (works with wrapped errors) + if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { return true - case nil, context.Canceled, context.DeadlineExceeded: + } + + // Check for context errors (works with wrapped errors) + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return false - case pool.ErrPoolTimeout: + } + + // Check for pool timeout (works with wrapped errors) + if errors.Is(err, pool.ErrPoolTimeout) { // connection pool timeout, increase retries. #3289 return true } - if v, ok := err.(timeoutError); ok { - if v.Timeout() { + // Check for timeout errors (works with wrapped errors) + if isTimeout, hasTimeoutFlag := isTimeoutError(err); isTimeout { + if hasTimeoutFlag { return retryTimeout } return true @@ -115,23 +142,37 @@ func shouldRetry(err error, retryTimeout bool) bool { if strings.HasPrefix(s, "TRYAGAIN ") { return true } + if strings.HasPrefix(s, "MASTERDOWN ") { + return true + } return false } func isRedisError(err error) bool { - _, ok := err.(proto.RedisError) - return ok + // Check if error implements the Error interface (works with wrapped errors) + var redisErr Error + if errors.As(err, &redisErr) { + return true + } + // Also check for proto.RedisError specifically + var protoRedisErr proto.RedisError + return errors.As(err, &protoRedisErr) } func isBadConn(err error, allowTimeout bool, addr string) bool { - switch err { - case nil: - return false - case context.Canceled, context.DeadlineExceeded: - return true - case pool.ErrConnUnusableTimeout: - return true + if err == nil { + return false + } + + // Check for context errors (works with wrapped errors) + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return true + } + + // Check for pool timeout errors (works with wrapped errors) + if errors.Is(err, pool.ErrConnUnusableTimeout) { + return true } if isRedisError(err) { @@ -151,7 +192,9 @@ func isBadConn(err error, allowTimeout bool, addr string) bool { } if allowTimeout { - if netErr, ok := err.(net.Error); ok && netErr.Timeout() { + // Check for network timeout errors (works with wrapped errors) + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { return false } } diff --git a/error_wrapping_test.go b/error_wrapping_test.go index 62ef4f07f9..9790b2d8c8 100644 --- a/error_wrapping_test.go +++ b/error_wrapping_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "io" + "strings" "testing" "github.com/redis/go-redis/v9" @@ -443,17 +445,198 @@ func TestCustomErrorTypeWrapping(t *testing.T) { } } -// Helper function to check if a string contains a substring -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && findSubstring(s, substr)) +// TestTimeoutErrorWrapping tests that timeout errors work correctly when wrapped +func TestTimeoutErrorWrapping(t *testing.T) { + // Test 1: Wrapped timeoutError interface + t.Run("Wrapped timeoutError with Timeout()=true", func(t *testing.T) { + timeoutErr := &testTimeoutError{timeout: true, msg: "i/o timeout"} + wrappedErr := fmt.Errorf("hook wrapper: %w", timeoutErr) + doubleWrappedErr := fmt.Errorf("another wrapper: %w", wrappedErr) + + // Should NOT retry when retryTimeout=false + if redis.ShouldRetry(doubleWrappedErr, false) { + t.Errorf("Should not retry timeout error when retryTimeout=false") + } + + // Should retry when retryTimeout=true + if !redis.ShouldRetry(doubleWrappedErr, true) { + t.Errorf("Should retry timeout error when retryTimeout=true") + } + }) + + // Test 2: Wrapped timeoutError with Timeout()=false + t.Run("Wrapped timeoutError with Timeout()=false", func(t *testing.T) { + timeoutErr := &testTimeoutError{timeout: false, msg: "connection error"} + wrappedErr := fmt.Errorf("hook wrapper: %w", timeoutErr) + + // Should always retry when Timeout()=false + if !redis.ShouldRetry(wrappedErr, false) { + t.Errorf("Should retry non-timeout error even when retryTimeout=false") + } + if !redis.ShouldRetry(wrappedErr, true) { + t.Errorf("Should retry non-timeout error when retryTimeout=true") + } + }) + + // Test 3: Wrapped net.Error with Timeout()=true + t.Run("Wrapped net.Error", func(t *testing.T) { + netErr := &testNetError{timeout: true, temporary: true, msg: "network timeout"} + wrappedErr := fmt.Errorf("hook context: %w", netErr) + + // Should respect retryTimeout parameter + if redis.ShouldRetry(wrappedErr, false) { + t.Errorf("Should not retry network timeout when retryTimeout=false") + } + if !redis.ShouldRetry(wrappedErr, true) { + t.Errorf("Should retry network timeout when retryTimeout=true") + } + }) + + // Test 4: Multiple levels of wrapping + t.Run("Multiple levels of wrapping", func(t *testing.T) { + timeoutErr := &testTimeoutError{timeout: true, msg: "timeout"} + customErr := &AppError{ + Code: "TIMEOUT_ERROR", + Message: "Operation timed out", + RequestID: "req-timeout-123", + Err: timeoutErr, + } + wrappedErr := fmt.Errorf("hook wrapper: %w", customErr) + + // Should still detect timeout through multiple wrappers + if redis.ShouldRetry(wrappedErr, false) { + t.Errorf("Should not retry timeout through custom error when retryTimeout=false") + } + if !redis.ShouldRetry(wrappedErr, true) { + t.Errorf("Should retry timeout through custom error when retryTimeout=true") + } + + // Should be able to extract custom error + var appErr *AppError + if !errors.As(wrappedErr, &appErr) { + t.Errorf("Should be able to extract AppError from wrapped error") + } + }) +} + +// testTimeoutError implements the timeoutError interface for testing +type testTimeoutError struct { + timeout bool + msg string +} + +func (e *testTimeoutError) Error() string { + return e.msg +} + +func (e *testTimeoutError) Timeout() bool { + return e.timeout +} + +// testNetError implements net.Error for testing +type testNetError struct { + timeout bool + temporary bool + msg string +} + +func (e *testNetError) Error() string { + return e.msg } -func findSubstring(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true +func (e *testNetError) Timeout() bool { + return e.timeout +} + +func (e *testNetError) Temporary() bool { + return e.temporary +} + +// TestContextErrorWrapping tests that context errors work correctly when wrapped +func TestContextErrorWrapping(t *testing.T) { + t.Run("Wrapped context.Canceled", func(t *testing.T) { + wrappedErr := fmt.Errorf("operation failed: %w", context.Canceled) + doubleWrappedErr := fmt.Errorf("hook wrapper: %w", wrappedErr) + + // Should NOT retry + if redis.ShouldRetry(doubleWrappedErr, false) { + t.Errorf("Should not retry wrapped context.Canceled") } - } - return false + if redis.ShouldRetry(doubleWrappedErr, true) { + t.Errorf("Should not retry wrapped context.Canceled even with retryTimeout=true") + } + }) + + t.Run("Wrapped context.DeadlineExceeded", func(t *testing.T) { + wrappedErr := fmt.Errorf("timeout: %w", context.DeadlineExceeded) + doubleWrappedErr := fmt.Errorf("hook wrapper: %w", wrappedErr) + + // Should NOT retry + if redis.ShouldRetry(doubleWrappedErr, false) { + t.Errorf("Should not retry wrapped context.DeadlineExceeded") + } + if redis.ShouldRetry(doubleWrappedErr, true) { + t.Errorf("Should not retry wrapped context.DeadlineExceeded even with retryTimeout=true") + } + }) +} + +// TestIOErrorWrapping tests that io errors work correctly when wrapped +func TestIOErrorWrapping(t *testing.T) { + t.Run("Wrapped io.EOF", func(t *testing.T) { + wrappedErr := fmt.Errorf("read failed: %w", io.EOF) + doubleWrappedErr := fmt.Errorf("hook wrapper: %w", wrappedErr) + + // Should retry + if !redis.ShouldRetry(doubleWrappedErr, false) { + t.Errorf("Should retry wrapped io.EOF") + } + }) + + t.Run("Wrapped io.ErrUnexpectedEOF", func(t *testing.T) { + wrappedErr := fmt.Errorf("read failed: %w", io.ErrUnexpectedEOF) + + // Should retry + if !redis.ShouldRetry(wrappedErr, false) { + t.Errorf("Should retry wrapped io.ErrUnexpectedEOF") + } + }) +} + +// TestPoolErrorWrapping tests that pool errors work correctly when wrapped +func TestPoolErrorWrapping(t *testing.T) { + t.Run("Wrapped pool.ErrPoolTimeout", func(t *testing.T) { + wrappedErr := fmt.Errorf("connection failed: %w", redis.ErrPoolTimeout) + doubleWrappedErr := fmt.Errorf("hook wrapper: %w", wrappedErr) + + // Should retry + if !redis.ShouldRetry(doubleWrappedErr, false) { + t.Errorf("Should retry wrapped pool.ErrPoolTimeout") + } + }) +} + +// TestRedisErrorWrapping tests that RedisError detection works with wrapped errors +func TestRedisErrorWrapping(t *testing.T) { + t.Run("Wrapped proto.RedisError", func(t *testing.T) { + redisErr := proto.RedisError("ERR something went wrong") + wrappedErr := fmt.Errorf("command failed: %w", redisErr) + doubleWrappedErr := fmt.Errorf("hook wrapper: %w", wrappedErr) + + // Create a command and set the wrapped error + cmd := redis.NewStatusCmd(context.Background(), "GET", "key") + cmd.SetErr(doubleWrappedErr) + + // The error should still be recognized as a Redis error + // This is tested indirectly through the typed error system + if !strings.Contains(cmd.Err().Error(), "ERR something went wrong") { + t.Errorf("Error message not preserved through wrapping") + } + }) +} + +// Helper function to check if a string contains a substring +func contains(s, substr string) bool { + return strings.Contains(s, substr) } diff --git a/internal/proto/redis_errors.go b/internal/proto/redis_errors.go index 5fdf133c19..f918f8a4a5 100644 --- a/internal/proto/redis_errors.go +++ b/internal/proto/redis_errors.go @@ -198,6 +198,11 @@ func IsLoadingError(err error) bool { if errors.As(err, &loadingErr) { return true } + // Check if wrapped error is a RedisError with LOADING prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "LOADING ") { + return true + } // Fallback to string checking for backward compatibility return strings.HasPrefix(err.Error(), "LOADING ") } @@ -211,6 +216,11 @@ func IsReadOnlyError(err error) bool { if errors.As(err, &readOnlyErr) { return true } + // Check if wrapped error is a RedisError with READONLY prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "READONLY ") { + return true + } // Fallback to string checking for backward compatibility return strings.HasPrefix(err.Error(), "READONLY ") } @@ -268,6 +278,11 @@ func IsClusterDownError(err error) bool { if errors.As(err, &clusterDownErr) { return true } + // Check if wrapped error is a RedisError with CLUSTERDOWN prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "CLUSTERDOWN ") { + return true + } // Fallback to string checking for backward compatibility return strings.HasPrefix(err.Error(), "CLUSTERDOWN ") } @@ -281,6 +296,11 @@ func IsTryAgainError(err error) bool { if errors.As(err, &tryAgainErr) { return true } + // Check if wrapped error is a RedisError with TRYAGAIN prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "TRYAGAIN ") { + return true + } // Fallback to string checking for backward compatibility return strings.HasPrefix(err.Error(), "TRYAGAIN ") } @@ -294,6 +314,11 @@ func IsMasterDownError(err error) bool { if errors.As(err, &masterDownErr) { return true } + // Check if wrapped error is a RedisError with MASTERDOWN prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "MASTERDOWN ") { + return true + } // Fallback to string checking for backward compatibility return strings.HasPrefix(err.Error(), "MASTERDOWN ") } @@ -307,6 +332,11 @@ func IsMaxClientsError(err error) bool { if errors.As(err, &maxClientsErr) { return true } + // Check if wrapped error is a RedisError with max clients prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "ERR max number of clients reached") { + return true + } // Fallback to string checking for backward compatibility return strings.HasPrefix(err.Error(), "ERR max number of clients reached") } From c4dc1eee4f564a03362d68646d2ef2bfa5b4762c Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Wed, 19 Nov 2025 15:02:59 +0200 Subject: [PATCH 08/12] use unwrapping of errors in push for consistency --- push/errors.go | 18 +++++--- push/processor_unit_test.go | 83 +++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 6 deletions(-) diff --git a/push/errors.go b/push/errors.go index 9eda92ddd6..c10c98aa86 100644 --- a/push/errors.go +++ b/push/errors.go @@ -145,25 +145,31 @@ func IsHandlerNilError(err error) bool { return errors.Is(err, ErrHandlerNil) } -// IsHandlerExistsError checks if an error is due to attempting to overwrite an existing handler +// IsHandlerExistsError checks if an error is due to attempting to overwrite an existing handler. +// This function works correctly even when the error is wrapped. func IsHandlerExistsError(err error) bool { - if handlerErr, ok := err.(*HandlerError); ok { + var handlerErr *HandlerError + if errors.As(err, &handlerErr) { return handlerErr.Operation == ProcessorOperationRegister && handlerErr.Reason == ReasonHandlerExists } return false } -// IsProtectedHandlerError checks if an error is due to attempting to unregister a protected handler +// IsProtectedHandlerError checks if an error is due to attempting to unregister a protected handler. +// This function works correctly even when the error is wrapped. func IsProtectedHandlerError(err error) bool { - if handlerErr, ok := err.(*HandlerError); ok { + var handlerErr *HandlerError + if errors.As(err, &handlerErr) { return handlerErr.Operation == ProcessorOperationUnregister && handlerErr.Reason == ReasonHandlerProtected } return false } -// IsVoidProcessorError checks if an error is due to void processor operations +// IsVoidProcessorError checks if an error is due to void processor operations. +// This function works correctly even when the error is wrapped. func IsVoidProcessorError(err error) bool { - if procErr, ok := err.(*ProcessorError); ok { + var procErr *ProcessorError + if errors.As(err, &procErr) { return procErr.ProcessorType == ProcessorTypeVoidProcessor && procErr.Reason == ReasonPushNotificationsDisabled } return false diff --git a/push/processor_unit_test.go b/push/processor_unit_test.go index ce7990489f..5678977657 100644 --- a/push/processor_unit_test.go +++ b/push/processor_unit_test.go @@ -2,6 +2,7 @@ package push import ( "context" + "fmt" "testing" ) @@ -313,3 +314,85 @@ func (h *UnitTestHandler) Reset() { h.lastNotification = nil h.errorToReturn = nil } + +// TestErrorWrapping tests that error checking functions work with wrapped errors +func TestErrorWrapping(t *testing.T) { + t.Run("IsHandlerExistsError with wrapped error", func(t *testing.T) { + // Create a HandlerError + handlerErr := ErrHandlerExists("test-notification") + + // Wrap it + wrappedErr := fmt.Errorf("operation failed: %w", handlerErr) + doubleWrappedErr := fmt.Errorf("context: %w", wrappedErr) + + // Should still be detected through wrapping + if !IsHandlerExistsError(doubleWrappedErr) { + t.Errorf("IsHandlerExistsError should detect wrapped error") + } + + // Verify it doesn't match other error types + if IsProtectedHandlerError(doubleWrappedErr) { + t.Errorf("IsProtectedHandlerError should not match handler exists error") + } + }) + + t.Run("IsProtectedHandlerError with wrapped error", func(t *testing.T) { + // Create a protected handler error + protectedErr := ErrProtectedHandler("protected-notification") + + // Wrap it + wrappedErr := fmt.Errorf("unregister failed: %w", protectedErr) + + // Should still be detected through wrapping + if !IsProtectedHandlerError(wrappedErr) { + t.Errorf("IsProtectedHandlerError should detect wrapped error") + } + + // Verify it doesn't match other error types + if IsHandlerExistsError(wrappedErr) { + t.Errorf("IsHandlerExistsError should not match protected handler error") + } + }) + + t.Run("IsVoidProcessorError with wrapped error", func(t *testing.T) { + // Create a void processor error + voidErr := ErrVoidProcessorRegister("test-notification") + + // Wrap it multiple times + wrappedErr := fmt.Errorf("register failed: %w", voidErr) + doubleWrappedErr := fmt.Errorf("processor error: %w", wrappedErr) + + // Should still be detected through wrapping + if !IsVoidProcessorError(doubleWrappedErr) { + t.Errorf("IsVoidProcessorError should detect wrapped error") + } + }) + + t.Run("IsHandlerNilError with wrapped error", func(t *testing.T) { + // Wrap the nil handler error + wrappedErr := fmt.Errorf("validation failed: %w", ErrHandlerNil) + + // Should still be detected through wrapping + if !IsHandlerNilError(wrappedErr) { + t.Errorf("IsHandlerNilError should detect wrapped error") + } + }) + + t.Run("Error functions return false for non-matching errors", func(t *testing.T) { + // Create a different error + otherErr := fmt.Errorf("some other error") + + if IsHandlerExistsError(otherErr) { + t.Errorf("IsHandlerExistsError should return false for non-matching error") + } + if IsProtectedHandlerError(otherErr) { + t.Errorf("IsProtectedHandlerError should return false for non-matching error") + } + if IsVoidProcessorError(otherErr) { + t.Errorf("IsVoidProcessorError should return false for non-matching error") + } + if IsHandlerNilError(otherErr) { + t.Errorf("IsHandlerNilError should return false for non-matching error") + } + }) +} From 3b23fcec8f047de3333956cc65c52736bfdf4ef3 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Wed, 19 Nov 2025 15:20:36 +0200 Subject: [PATCH 09/12] add common error types --- README.md | 65 +++++++++++- error.go | 27 +++++ error_wrapping_test.go | 86 +++++++++++++++ internal/proto/redis_errors.go | 157 ++++++++++++++++++++++++++-- internal/proto/redis_errors_test.go | 79 +++++++++++++- 5 files changed, 404 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index dc3808d81b..5aaa79e5d8 100644 --- a/README.md +++ b/README.md @@ -434,14 +434,23 @@ res, err := rdb.Do(ctx, "set", "key", "value").Result() go-redis provides typed error checking functions for common Redis errors: ```go +// Cluster and replication errors redis.IsLoadingError(err) // Redis is loading the dataset redis.IsReadOnlyError(err) // Write to read-only replica redis.IsClusterDownError(err) // Cluster is down redis.IsTryAgainError(err) // Command should be retried redis.IsMasterDownError(err) // Master is down -redis.IsMaxClientsError(err) // Maximum clients reached redis.IsMovedError(err) // Returns (address, true) if key moved redis.IsAskError(err) // Returns (address, true) if key being migrated + +// Connection and resource errors +redis.IsMaxClientsError(err) // Maximum clients reached +redis.IsAuthError(err) // Authentication failed (NOAUTH, WRONGPASS, unauthenticated) +redis.IsPermissionError(err) // Permission denied (NOPERM) +redis.IsOOMError(err) // Out of memory (OOM) + +// Transaction errors +redis.IsExecAbortError(err) // Transaction aborted (EXECABORT) ``` ### Error Wrapping in Hooks @@ -500,6 +509,60 @@ wrappedErr := fmt.Errorf("context: %w", err) cmd.SetErr(wrappedErr) ``` +### Pipeline Hook Example + +For pipeline operations, use `ProcessPipelineHook`: + +```go +type PipelineLoggingHook struct{} + +func (h PipelineLoggingHook) DialHook(next redis.DialHook) redis.DialHook { + return next +} + +func (h PipelineLoggingHook) ProcessHook(next redis.ProcessHook) redis.ProcessHook { + return next +} + +func (h PipelineLoggingHook) ProcessPipelineHook(next redis.ProcessPipelineHook) redis.ProcessPipelineHook { + return func(ctx context.Context, cmds []redis.Cmder) error { + start := time.Now() + err := next(ctx, cmds) + duration := time.Since(start) + + // Log pipeline execution + log.Printf("Pipeline executed %d commands in %v", len(cmds), duration) + + // Check for errors in individual commands + for _, cmd := range cmds { + if cmdErr := cmd.Err(); cmdErr != nil { + // Wrap individual command errors if needed + if redis.IsAuthError(cmdErr) { + log.Printf("Auth error in pipeline command %s: %v", cmd.Name(), cmdErr) + } else if redis.IsPermissionError(cmdErr) { + log.Printf("Permission error in pipeline command %s: %v", cmd.Name(), cmdErr) + } + + // Optionally wrap the error + wrappedErr := fmt.Errorf("pipeline cmd %s failed: %w", cmd.Name(), cmdErr) + cmd.SetErr(wrappedErr) + } + } + + return err + } +} + +// Register the hook +rdb.AddHook(PipelineLoggingHook{}) + +// Use pipeline - errors are still properly typed +pipe := rdb.Pipeline() +pipe.Set(ctx, "key1", "value1", 0) +pipe.Get(ctx, "key2") +_, err := pipe.Exec(ctx) +``` + ## Run the test Recommended to use Docker, just need to run: diff --git a/error.go b/error.go index 033b235bdd..12b5604dfd 100644 --- a/error.go +++ b/error.go @@ -315,6 +315,33 @@ func IsAskError(err error) (addr string, ok bool) { return "", false } +// IsAuthError checks if an error is a Redis authentication error, even if wrapped. +// Authentication errors occur when: +// - NOAUTH: Redis requires authentication but none was provided +// - WRONGPASS: Redis authentication failed due to incorrect password +// - unauthenticated: Error returned when password changed +func IsAuthError(err error) bool { + return proto.IsAuthError(err) +} + +// IsPermissionError checks if an error is a Redis permission error, even if wrapped. +// Permission errors (NOPERM) occur when a user does not have permission to execute a command. +func IsPermissionError(err error) bool { + return proto.IsPermissionError(err) +} + +// IsExecAbortError checks if an error is a Redis EXECABORT error, even if wrapped. +// EXECABORT errors occur when a transaction is aborted. +func IsExecAbortError(err error) bool { + return proto.IsExecAbortError(err) +} + +// IsOOMError checks if an error is a Redis OOM (Out Of Memory) error, even if wrapped. +// OOM errors occur when Redis is out of memory. +func IsOOMError(err error) bool { + return proto.IsOOMError(err) +} + //------------------------------------------------------------------------------ type timeoutError interface { diff --git a/error_wrapping_test.go b/error_wrapping_test.go index 9790b2d8c8..574d13797a 100644 --- a/error_wrapping_test.go +++ b/error_wrapping_test.go @@ -640,3 +640,89 @@ func contains(s, substr string) bool { return strings.Contains(s, substr) } +func TestAuthErrorWrapping(t *testing.T) { + t.Run("Wrapped NOAUTH error", func(t *testing.T) { + // Create an auth error + authErr := proto.NewAuthError("NOAUTH Authentication required") + + // Wrap it + wrappedErr := fmt.Errorf("hook: %w", authErr) + + // Should still be detected + if !redis.IsAuthError(wrappedErr) { + t.Errorf("IsAuthError should detect wrapped NOAUTH error") + } + }) + + t.Run("Wrapped WRONGPASS error", func(t *testing.T) { + // Create an auth error + authErr := proto.NewAuthError("WRONGPASS invalid username-password pair") + + // Wrap it multiple times + wrappedErr := fmt.Errorf("connection error: %w", authErr) + doubleWrappedErr := fmt.Errorf("client error: %w", wrappedErr) + + // Should still be detected + if !redis.IsAuthError(doubleWrappedErr) { + t.Errorf("IsAuthError should detect double-wrapped WRONGPASS error") + } + }) + + t.Run("Wrapped unauthenticated error", func(t *testing.T) { + // Create an auth error + authErr := proto.NewAuthError("ERR unauthenticated") + + // Wrap it + wrappedErr := fmt.Errorf("hook: %w", authErr) + + // Should still be detected + if !redis.IsAuthError(wrappedErr) { + t.Errorf("IsAuthError should detect wrapped unauthenticated error") + } + }) +} + +func TestPermissionErrorWrapping(t *testing.T) { + t.Run("Wrapped NOPERM error", func(t *testing.T) { + // Create a permission error + permErr := proto.NewPermissionError("NOPERM this user has no permissions to run the 'flushdb' command") + + // Wrap it + wrappedErr := fmt.Errorf("hook: %w", permErr) + + // Should still be detected + if !redis.IsPermissionError(wrappedErr) { + t.Errorf("IsPermissionError should detect wrapped NOPERM error") + } + }) +} + +func TestExecAbortErrorWrapping(t *testing.T) { + t.Run("Wrapped EXECABORT error", func(t *testing.T) { + // Create an EXECABORT error + execAbortErr := proto.NewExecAbortError("EXECABORT Transaction discarded because of previous errors") + + // Wrap it + wrappedErr := fmt.Errorf("hook: %w", execAbortErr) + + // Should still be detected + if !redis.IsExecAbortError(wrappedErr) { + t.Errorf("IsExecAbortError should detect wrapped EXECABORT error") + } + }) +} + +func TestOOMErrorWrapping(t *testing.T) { + t.Run("Wrapped OOM error", func(t *testing.T) { + // Create an OOM error + oomErr := proto.NewOOMError("OOM command not allowed when used memory > 'maxmemory'") + + // Wrap it + wrappedErr := fmt.Errorf("hook: %w", oomErr) + + // Should still be detected + if !redis.IsOOMError(wrappedErr) { + t.Errorf("IsOOMError should detect wrapped OOM error") + } + }) +} diff --git a/internal/proto/redis_errors.go b/internal/proto/redis_errors.go index f918f8a4a5..f553e2f962 100644 --- a/internal/proto/redis_errors.go +++ b/internal/proto/redis_errors.go @@ -148,6 +148,70 @@ func NewMaxClientsError(msg string) *MaxClientsError { return &MaxClientsError{msg: msg} } +// AuthError is returned when authentication fails. +type AuthError struct { + msg string +} + +func (e *AuthError) Error() string { + return e.msg +} + +func (e *AuthError) RedisError() {} + +// NewAuthError creates a new AuthError with the given message. +func NewAuthError(msg string) *AuthError { + return &AuthError{msg: msg} +} + +// PermissionError is returned when a user lacks required permissions. +type PermissionError struct { + msg string +} + +func (e *PermissionError) Error() string { + return e.msg +} + +func (e *PermissionError) RedisError() {} + +// NewPermissionError creates a new PermissionError with the given message. +func NewPermissionError(msg string) *PermissionError { + return &PermissionError{msg: msg} +} + +// ExecAbortError is returned when a transaction is aborted. +type ExecAbortError struct { + msg string +} + +func (e *ExecAbortError) Error() string { + return e.msg +} + +func (e *ExecAbortError) RedisError() {} + +// NewExecAbortError creates a new ExecAbortError with the given message. +func NewExecAbortError(msg string) *ExecAbortError { + return &ExecAbortError{msg: msg} +} + +// OOMError is returned when Redis is out of memory. +type OOMError struct { + msg string +} + +func (e *OOMError) Error() string { + return e.msg +} + +func (e *OOMError) RedisError() {} + +// NewOOMError creates a new OOMError with the given message. +func NewOOMError(msg string) *OOMError { + return &OOMError{msg: msg} +} + // parseTypedRedisError parses a Redis error message and returns a typed error if applicable. // This function maintains backward compatibility by keeping the same error messages. func parseTypedRedisError(msg string) error { @@ -173,6 +237,14 @@ func parseTypedRedisError(msg string) error { return NewMasterDownError(msg) case msg == "ERR max number of clients reached": return NewMaxClientsError(msg) + case strings.HasPrefix(msg, "NOAUTH "), strings.HasPrefix(msg, "WRONGPASS "), strings.Contains(msg, "unauthenticated"): + return NewAuthError(msg) + case strings.HasPrefix(msg, "NOPERM "): + return NewPermissionError(msg) + case strings.HasPrefix(msg, "EXECABORT "): + return NewExecAbortError(msg) + case strings.HasPrefix(msg, "OOM "): + return NewOOMError(msg) default: // Return generic RedisError for unknown error types return RedisError(msg) @@ -200,7 +272,7 @@ func IsLoadingError(err error) bool { } // Check if wrapped error is a RedisError with LOADING prefix var redisErr RedisError - if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "LOADING ") { + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "LOADING ") { return true } // Fallback to string checking for backward compatibility @@ -218,7 +290,7 @@ func IsReadOnlyError(err error) bool { } // Check if wrapped error is a RedisError with READONLY prefix var redisErr RedisError - if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "READONLY ") { + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "READONLY ") { return true } // Fallback to string checking for backward compatibility @@ -280,7 +352,7 @@ func IsClusterDownError(err error) bool { } // Check if wrapped error is a RedisError with CLUSTERDOWN prefix var redisErr RedisError - if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "CLUSTERDOWN ") { + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "CLUSTERDOWN ") { return true } // Fallback to string checking for backward compatibility @@ -298,7 +370,7 @@ func IsTryAgainError(err error) bool { } // Check if wrapped error is a RedisError with TRYAGAIN prefix var redisErr RedisError - if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "TRYAGAIN ") { + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "TRYAGAIN ") { return true } // Fallback to string checking for backward compatibility @@ -316,7 +388,7 @@ func IsMasterDownError(err error) bool { } // Check if wrapped error is a RedisError with MASTERDOWN prefix var redisErr RedisError - if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "MASTERDOWN ") { + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "MASTERDOWN ") { return true } // Fallback to string checking for backward compatibility @@ -334,10 +406,83 @@ func IsMaxClientsError(err error) bool { } // Check if wrapped error is a RedisError with max clients prefix var redisErr RedisError - if errors.As(err, &redisErr) && strings.HasPrefix(string(redisErr), "ERR max number of clients reached") { + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "ERR max number of clients reached") { return true } // Fallback to string checking for backward compatibility return strings.HasPrefix(err.Error(), "ERR max number of clients reached") } +// IsAuthError checks if an error is an AuthError, even if wrapped. +func IsAuthError(err error) bool { + if err == nil { + return false + } + var authErr *AuthError + if errors.As(err, &authErr) { + return true + } + // Check if wrapped error is a RedisError with auth error prefix + var redisErr RedisError + if errors.As(err, &redisErr) { + s := redisErr.Error() + return strings.HasPrefix(s, "NOAUTH ") || strings.HasPrefix(s, "WRONGPASS ") || strings.Contains(s, "unauthenticated") + } + // Fallback to string checking for backward compatibility + s := err.Error() + return strings.HasPrefix(s, "NOAUTH ") || strings.HasPrefix(s, "WRONGPASS ") || strings.Contains(s, "unauthenticated") +} + +// IsPermissionError checks if an error is a PermissionError, even if wrapped. +func IsPermissionError(err error) bool { + if err == nil { + return false + } + var permErr *PermissionError + if errors.As(err, &permErr) { + return true + } + // Check if wrapped error is a RedisError with NOPERM prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "NOPERM ") { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "NOPERM ") +} + +// IsExecAbortError checks if an error is an ExecAbortError, even if wrapped. +func IsExecAbortError(err error) bool { + if err == nil { + return false + } + var execAbortErr *ExecAbortError + if errors.As(err, &execAbortErr) { + return true + } + // Check if wrapped error is a RedisError with EXECABORT prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "EXECABORT ") { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "EXECABORT ") +} + +// IsOOMError checks if an error is an OOMError, even if wrapped. +func IsOOMError(err error) bool { + if err == nil { + return false + } + var oomErr *OOMError + if errors.As(err, &oomErr) { + return true + } + // Check if wrapped error is a RedisError with OOM prefix + var redisErr RedisError + if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "OOM ") { + return true + } + // Fallback to string checking for backward compatibility + return strings.HasPrefix(err.Error(), "OOM ") +} diff --git a/internal/proto/redis_errors_test.go b/internal/proto/redis_errors_test.go index c6ab081fa7..452a452435 100644 --- a/internal/proto/redis_errors_test.go +++ b/internal/proto/redis_errors_test.go @@ -90,6 +90,48 @@ func TestTypedRedisErrors(t *testing.T) { expectedMsg: "ERR max number of clients reached", checkFunc: IsMaxClientsError, }, + { + name: "NOAUTH error", + errorMsg: "NOAUTH Authentication required", + expectedType: &AuthError{}, + expectedMsg: "NOAUTH Authentication required", + checkFunc: IsAuthError, + }, + { + name: "WRONGPASS error", + errorMsg: "WRONGPASS invalid username-password pair", + expectedType: &AuthError{}, + expectedMsg: "WRONGPASS invalid username-password pair", + checkFunc: IsAuthError, + }, + { + name: "unauthenticated error", + errorMsg: "ERR unauthenticated", + expectedType: &AuthError{}, + expectedMsg: "ERR unauthenticated", + checkFunc: IsAuthError, + }, + { + name: "NOPERM error", + errorMsg: "NOPERM this user has no permissions to run the 'flushdb' command", + expectedType: &PermissionError{}, + expectedMsg: "NOPERM this user has no permissions to run the 'flushdb' command", + checkFunc: IsPermissionError, + }, + { + name: "EXECABORT error", + errorMsg: "EXECABORT Transaction discarded because of previous errors", + expectedType: &ExecAbortError{}, + expectedMsg: "EXECABORT Transaction discarded because of previous errors", + checkFunc: IsExecAbortError, + }, + { + name: "OOM error", + errorMsg: "OOM command not allowed when used memory > 'maxmemory'", + expectedType: &OOMError{}, + expectedMsg: "OOM command not allowed when used memory > 'maxmemory'", + checkFunc: IsOOMError, + }, } for _, tt := range tests { @@ -159,6 +201,36 @@ func TestWrappedTypedErrors(t *testing.T) { errorMsg: "ERR max number of clients reached", checkFunc: IsMaxClientsError, }, + { + name: "Wrapped NOAUTH error", + errorMsg: "NOAUTH Authentication required", + checkFunc: IsAuthError, + }, + { + name: "Wrapped WRONGPASS error", + errorMsg: "WRONGPASS invalid username-password pair", + checkFunc: IsAuthError, + }, + { + name: "Wrapped unauthenticated error", + errorMsg: "ERR unauthenticated", + checkFunc: IsAuthError, + }, + { + name: "Wrapped NOPERM error", + errorMsg: "NOPERM this user has no permissions to run the 'flushdb' command", + checkFunc: IsPermissionError, + }, + { + name: "Wrapped EXECABORT error", + errorMsg: "EXECABORT Transaction discarded because of previous errors", + checkFunc: IsExecAbortError, + }, + { + name: "Wrapped OOM error", + errorMsg: "OOM command not allowed when used memory > 'maxmemory'", + checkFunc: IsOOMError, + }, } for _, tt := range tests { @@ -261,8 +333,8 @@ func TestGenericRedisError(t *testing.T) { errorMsg: "WRONGTYPE Operation against a key holding the wrong kind of value", }, { - name: "NOAUTH error", - errorMsg: "NOAUTH Authentication required", + name: "BUSYKEY error", + errorMsg: "BUSYKEY Target key name already exists", }, } @@ -282,7 +354,8 @@ func TestGenericRedisError(t *testing.T) { // Should not match any typed error checks if IsLoadingError(err) || IsReadOnlyError(err) || IsClusterDownError(err) || - IsTryAgainError(err) || IsMasterDownError(err) || IsMaxClientsError(err) { + IsTryAgainError(err) || IsMasterDownError(err) || IsMaxClientsError(err) || + IsAuthError(err) || IsPermissionError(err) || IsExecAbortError(err) || IsOOMError(err) { t.Errorf("Generic error incorrectly matched a typed error check") } }) From dd9ca52c9d2abfbc7375291be27100ab0babbf49 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Wed, 19 Nov 2025 15:36:00 +0200 Subject: [PATCH 10/12] fix test --- acl_commands_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/acl_commands_test.go b/acl_commands_test.go index d2cb17a752..5619ef310a 100644 --- a/acl_commands_test.go +++ b/acl_commands_test.go @@ -306,7 +306,7 @@ var _ = Describe("ACL permissions", Label("NonRedisEnterprise"), func() { // no perm for dropindex err = c.FTDropIndex(ctx, "txt").Err() - Expect(err).ToNot(BeEmpty()) + Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("NOPERM")) // json set and get have perm @@ -315,7 +315,7 @@ var _ = Describe("ACL permissions", Label("NonRedisEnterprise"), func() { // no perm for json clear err = c.JSONClear(ctx, "foo", "$").Err() - Expect(err).ToNot(BeEmpty()) + Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("NOPERM")) // perm for reserve @@ -323,7 +323,7 @@ var _ = Describe("ACL permissions", Label("NonRedisEnterprise"), func() { // no perm for info err = c.BFInfo(ctx, "bloom").Err() - Expect(err).ToNot(BeEmpty()) + Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("NOPERM")) // perm for cf.reserve @@ -338,7 +338,7 @@ var _ = Describe("ACL permissions", Label("NonRedisEnterprise"), func() { Expect(c.TSCreate(ctx, "tsts").Err()).NotTo(HaveOccurred()) // noperm for ts.info err = c.TSInfo(ctx, "tsts").Err() - Expect(err).ToNot(BeEmpty()) + Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("NOPERM")) Expect(client.FlushDB(ctx).Err()).NotTo(HaveOccurred()) From a85becc9811c44d32ce55273292f9e34af6019fc Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Wed, 19 Nov 2025 15:49:07 +0200 Subject: [PATCH 11/12] fix flaky test --- options.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/options.go b/options.go index e0dcb5eba6..9773e86f77 100644 --- a/options.go +++ b/options.go @@ -355,6 +355,10 @@ func (opt *Options) init() { opt.MaxRetryBackoff = 512 * time.Millisecond } + if opt.FailingTimeoutSeconds == 0 { + opt.FailingTimeoutSeconds = 15 + } + opt.MaintNotificationsConfig = opt.MaintNotificationsConfig.ApplyDefaultsWithPoolConfig(opt.PoolSize, opt.MaxActiveConns) // auto-detect endpoint type if not specified From 6b5a569900188f35281927195de431d3a1c18b37 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Wed, 19 Nov 2025 16:16:32 +0200 Subject: [PATCH 12/12] add comments in the example --- README.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 5aaa79e5d8..0c67d379c5 100644 --- a/README.md +++ b/README.md @@ -527,28 +527,33 @@ func (h PipelineLoggingHook) ProcessHook(next redis.ProcessHook) redis.ProcessHo func (h PipelineLoggingHook) ProcessPipelineHook(next redis.ProcessPipelineHook) redis.ProcessPipelineHook { return func(ctx context.Context, cmds []redis.Cmder) error { start := time.Now() + + // Execute the pipeline err := next(ctx, cmds) - duration := time.Since(start) - // Log pipeline execution + duration := time.Since(start) log.Printf("Pipeline executed %d commands in %v", len(cmds), duration) - // Check for errors in individual commands + // Process individual command errors + // Note: Individual command errors are already set on each cmd by the pipeline execution for _, cmd := range cmds { if cmdErr := cmd.Err(); cmdErr != nil { - // Wrap individual command errors if needed + // Check for specific error types using typed error functions if redis.IsAuthError(cmdErr) { log.Printf("Auth error in pipeline command %s: %v", cmd.Name(), cmdErr) } else if redis.IsPermissionError(cmdErr) { log.Printf("Permission error in pipeline command %s: %v", cmd.Name(), cmdErr) } - // Optionally wrap the error + // Optionally wrap individual command errors to add context + // The wrapped error preserves type information through errors.As() wrappedErr := fmt.Errorf("pipeline cmd %s failed: %w", cmd.Name(), cmdErr) cmd.SetErr(wrappedErr) } } + // Return the pipeline-level error (connection errors, etc.) + // You can wrap it if needed, or return it as-is return err } }