Pipeline does not propogate errors to all commands properly

[AvyChanna]

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 dd9a200 .

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]

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.

go-redis/redis.go

Lines 515 to 519 in 2d8fa02

	lastErr = c.withConn(ctx, func(ctx context.Context, cn *pool.Conn) error {
	var err error
	canRetry, err = p(ctx, cn, cmds)
	return err
	})

In ClusterClient, this seems to be the path that the errors get set if the client is closed;

go-redis/osscluster.go

Lines 1221 to 1224 in 2d8fa02

	if err := c.mapCmdsByNode(ctx, cmdsMap, cmds); err != nil {
	setCmdsErr(cmds, err)
	return err
	}

[ndyakov]

Thank you for reporting this. @seruman feel free to open a PR with the fix if you think you have narrowed this down or write a test that can be used to check the expected behaviour.

[sgp]

So, I'm going to try to fix this in anger in the next week or so because I've been bitten by this for over a year now and I'm tired of it.

One thing I want to do as a good citizen is to make a unit/integration test that tests the problem and validates the fix, but there are issues with running make test in the main branch that are maddening (some tests never run, the docker-compose.yml starts three of the five pods with arguments that fail, etc.). I'm going to open another issue to track this.

[sgp]

Opened #3490 to track integration / containerized test issues.

[tptodorov]

I experienced the same problem with github.com/redis/go-redis/v9 v9.13.0

	pipe := cache.redisClient.TxPipeline()

	// Set the entry data
	pipe.HSet(ctx, key, entryMap)

	// Set TTL if specified
	if ttl > 0 {
		pipe.PExpire(ctx, key, time.Duration(ttl)*time.Millisecond)
	} else {
		pipe.Persist(ctx, key)
	}

	// Execute the transaction
	cmdErrors, err := pipe.Exec(ctx)

All errors in cmdErrors contain the last error returned in the pipeline instead of errors for each command.

---

For comparison, here is what I get when I run the same commands from CLI

multi
OK
0.0.0.0:64687(TX)> hset abc 122 121
(error) OOM command not allowed when used memory > 'maxmemory'.
0.0.0.0:64687(TX)> exec
(error) EXECABORT Transaction discarded because of previous errors.

I need to be able to get to the actual error "OOM command not allowed when used memory > 'maxmemory'."

[ndyakov]

@tptodorov thank you for reporting and adding the redis cli example. It looks like the error on queueing the command is ignored if it is a redis error.

go-redis/redis.go

Line 707 in 65e1c22

if err := statusCmd.readReply(rd); err != nil && !isRedisError(err) {

You can check the potential fix in #3509

[ndyakov]

Just a note, that those are two separate problems, but both should be fixed in the same PR.

• The first one reported by @AvyChanna was related to first error which was not supposed to be retried, especially if it is a client error before being able to get a connection, to not be assigned to the commands.

• The second one, reported by @tptodorov, was related to TxPipeline which was ignoring errors when queueing commands.

Both are addressed in the PR.