go-redis icon indicating copy to clipboard operation
go-redis copied to clipboard

Context timeout not working on blocking operations in v9

Open notrobpike opened this issue 1 year ago • 1 comments

import (
	"context"
	"errors"
	"testing"
	"time"

	"github.com/alicebob/miniredis/v2"
	"github.com/redis/go-redis/v9"
)

func TestContext(t *testing.T) {
	s := miniredis.RunT(t)
	redisClient := redis.NewClient(&redis.Options{
		Network:               s.Server().Addr().Network(),
		Addr:                  s.Addr(),
		ContextTimeoutEnabled: true,
	})

	t.Run("cancel context", func(t *testing.T) {
		ctx, cancel := context.WithCancel(context.Background())
		go func() {
			time.Sleep(time.Millisecond * 50)
			cancel()
		}()

		_, err := redisClient.BLMove(ctx, "src", "dst", "RIGHT", "LEFT", 0).Result()

		//NOTREACHED
		// if this were reachable, this would probably fail like the test below
		if !errors.Is(err, context.Canceled) {
			t.Errorf("expected to receive Canceled, instead got %T", err)
		}
	})

	t.Run("timeout context", func(t *testing.T) {
		ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
		defer cancel()

		_, err := redisClient.BLMove(ctx, "src", "dst", "RIGHT", "LEFT", 0).Result()

		if !errors.Is(err, context.DeadlineExceeded) {
			t.Errorf("expected to receive DealineExceeded, instead got %T", err)
		}
	})
}

both of these test cases pass in v8, but fail in v9.

the first one is reported in #2556 but maybe this adds some color. the second one seems to me that Options.ContextTimeoutEnabled is intended to address.

notrobpike avatar Aug 25 '23 20:08 notrobpike

We've also been hit by this. The Options.ContextTimeoutEnabled only works if commands are provided with a non-zero timeout (which seems counter-intuitive given the context rather than parameter timeout should be chosen and doesn't respect cancellation, only deadlines).

One way of solving this could be as I've described in https://github.com/redis/go-redis/issues/2556#issuecomment-1896224021

vtermanis avatar Jan 17 '24 17:01 vtermanis