redis / go-redis

Redis Go client
https://redis.uptrace.dev
BSD 2-Clause "Simplified" License
19.96k stars 2.36k forks source link

Pipeline does not propogate errors to all commands properly #2871

Open AvyChanna opened 8 months ago

AvyChanna commented 8 months ago

When using a pipeline, previous versions (v8..) used to set error on all commands in a pipeline when it failed. go-redis v9 does not call setCmdsErr in some cases.

Expected Behavior

All commands in the pipeline should have err set properly and cmd.Err() should return the error instead of nil for all cmds

Current Behavior

Commands in a failed pipeline return "", nil on calling Result()

Steps to Reproduce

package main

import (
    "context"
    "fmt"

    "github.com/redis/go-redis/v9"
)

func main() {
    fmt.Print("Initializing Redis client... ")
    rdb := redis.NewClient(&redis.Options{
        Addr:       "localhost:6379",
    })

    // closing client here to emulate error
    rdb.Close()

    p := rdb.Pipeline()
    cmd1 := p.Get(context.Background(), "get1")
    cmd2 := p.Get(context.Background(), "get2")

    _, errPipe := p.Exec(context.Background())

    err1 := cmd1.Err()
    err2 := cmd2.Err()

    fmt.Printf("Errors = %s, %s, %s\n", errPipe, err1, err2)
}

Context (Environment)

I was trying to use cmds.Result() directly (ignoring return val of pipe.Exec()) and checking for errors in the result itself.

(I don't know if this is the intended behavior or a regression. So sorry if it's the former)

I did a bisect on the code and I could narrow it down to https://github.com/redis/go-redis/commit/dd9a200427d7c57e480d61833a7204b13b55acbd .

Output of commit#6327c52e60cabd2acb43dcacab389def4e3e9a5a
Errors = redis: client is closed, redis: client is closed, redis: client is closed

Output of commit#dd9a200427d7c57e480d61833a7204b13b55acbd
Errors = redis: client is closed, %!s(<nil>), %!s(<nil>)

Also, this happens on some types of clients only

// before
Errors NewClusterClient = redis: client is closed, redis: client is closed, redis: client is closed
Errors NewFailoverClient = redis: client is closed, redis: client is closed, redis: client is closed
Errors NewClient = redis: client is closed, redis: client is closed, redis: client is closed
Errors NewRing = redis: client is closed, redis: client is closed, redis: client is closed

// after
Errors NewClusterClient = redis: client is closed, redis: client is closed, redis: client is closed
Errors NewFailoverClient = redis: client is closed, %!s(<nil>), %!s(<nil>)
Errors NewClient = redis: client is closed, %!s(<nil>), %!s(<nil>)
Errors NewRing = redis: client is closed, redis: client is closed, redis: client is closed

Thanks

seruman commented 4 months ago

Observed the same exact behaviour, here's what I could find;

Seems like the closure here (p == pipelineProcessCmds | txPipelineProcessCmds) is never called if the client is closed; AFAIU it is expected for processer p to set command errors but it never gets called with a closed client.

https://github.com/redis/go-redis/blob/2d8fa02ac2b0cda80410674a4abad00ea35217e8/redis.go#L515-L519

In ClusterClient, this seems to be the path that the errors get set if the client is closed; https://github.com/redis/go-redis/blob/2d8fa02ac2b0cda80410674a4abad00ea35217e8/osscluster.go#L1221-L1224