Context cancel not working on blocking operations in v9

[FLAGLORD]

I noticed the #1196 (comment), and it seems that maintainers have fixed this.

But i use the alomost the same code given in the #1196 (comment):

client := redis.NewClient(&redis.Options{
		Addr: "localhost:6379",
	})

cancelCtx, cancelFn := context.WithCancel(context.Background())

wg := &sync.WaitGroup{}
wg.Add(1)

go func() {
	client.BRPop(cancelCtx, 0, "")
	wg.Done()
}()

time.Sleep(2 * time.Second)

cancelFn()
wg.Wait()

It still waits indefinitely due to the 0 timeout. I have no idea...

[FLAGLORD]

Is it related to #2276 ?

[monkey92t]

We made an attempt in #2455, to be honest, the experience was poor. It disrupted our usual process of executing commands. In my opinion, we need to put more thought into it.

[monkey92t]

I feel that the cost of #2455 is too high, and I don't think it's a good solution, even though it solves the problem.

[agorman]

Is there a reasonable solution to this problem other than setting a low timeout and checking the value of context manually? Seems like we don't have a choice but to poll which defeats the purpose of the blocking pop. It also means I have to wait up to a second for it to respond to a context being canceled.

Anyone have a nicer solution?

var value string
for {
	select {
	case <-ctx.Done():
		return nil, context.Canceled
	default:
	}

	result, err := client.BLPop(ctx, time.Second, key).Result()
	if err == nil {
		value = result[1]
		break
	}
}

[FLAGLORD]

Is there a reasonable solution to this problem other than setting a low timeout and checking the value of context manually? Seems like we don't have a choice but to poll which defeats the purpose of the blocking pop. It also means I have to wait up to a second for it to respond to a context being canceled.

Anyone have a nicer solution?

var value string
for {
	select {
	case <-ctx.Done():
		return nil, context.Canceled
	default:
	}

	result, err := client.BLPop(ctx, time.Second, key).Result()
	if err == nil {
		value = result[1]
		break
	}
}

If you hope that it returns immediately when being canceled, code snippet below works.

var value string
doneChan := make(chan struct{})

go func() {
	for {
		select {
		case <-ctx.Done():
			return
		default:
		}
		result, err := client.BLPop(ctx, time.Second, key).Result()
		if err == nil {
			value = result[1]
			// Use select to avoid goroutine leak
			select {
			case doneChan <- struct{}{}:
			default:
			}
			return
		}
	}
}()

select {
case <-ctx.Done():
	return nil, context.Canceled
case <-doneChan:
}

However, it seems weird and tricky. I do not like it.

[FLAGLORD]

Sadly my code snippet has side effects:
If the program successfully read result from redis, but it hasn't notified by writing to donnChan. At this time, ctx is canceled. It will return context.canceled but has retrieved value from redis...

[FLAGLORD]

It seems impossible that there could be a solution that has no side effects and responds to context cancel quickly. Considering that waiting for cancel and BLPop are independent from each other:

• If the program responds to cancel immediately, we have to use a goroutine to do BLPop, and a cancel can be sent while blocking(it means side effects);
• If we hope to avoid dropping BLPop results, we have to block in the main process and it means that we have to tolerate a delay in context cancel.

Currently we have to trade off according to our needs

[monkey92t]

Because conn.Read(), conn.Write() do not depend on ctx, but only use timeout to control.

[monkey92t]

We still want go-redis to maintain high performance and do not want to use multiple goroutines to pass signals through channels, which would make it more complex. We want the API to be simple like this:

client.Get() -> write cmd -> read response data.

[abemedia]

Because conn.Read(), conn.Write() do not depend on ctx, but only use timeout to control.

You could simply using context.AfterFunc to set the deadline:

done := make(chan struct{})
stop := context.AfterFunc(ctx, func() {
	conn.SetReadDeadline(time.Now())
	close(done)
})
n, err = conn.Read(b)
if !stop() {
	<-done // Wait for AfterFunc to complete.
	conn.SetReadDeadline(time.Time{})
	// Return `conn` to pool.
	return n, ctx.Err()
}

This might not work for calls with side-effects but would work nicely for things like XRead and it's a lot more lightweight than the solution in the PR above.

It could also be further optimised by using a sync.Pool for for the channels that are created for waiting.

[vtermanis]

(Talking about state at v9.4.0)

We're also affected by the lack context cancellation awareness in our use-case: Long running BLPOP & PSUBSCRIBE calls. There are obviously workarounds - but they're not ideal:

1. For PSUBSCRIBE, use Channel instead of ReceiveMessage/Receive
   1. 🟢 The channel responds to PubSub.Close()
   2. 🔴 Two extra routines are used: initHealthCheck & init(All|Msg)Chan
      • The health check is obviously of use if one wants automatic re-connections (but this retry behaviour is only available for pubsub, not other blocking commands)
2. For BLPOP (and any other blocking commands which support a Redis command timeout parameter) specify a short timeout
   1. 🟢 If context is cancelled, the connection continue to block only up to the configured timeout.
   2. 🔴 The connection still blocks until the timeout
   3. 🔴 A low timeout increases unnecessary repeated polling and extra application logic (a while-true loop is needed)

---

We still want go-redis to maintain high performance and do not want to use multiple go-routines to pass signals through channels, which would make it more complex. We want the API to be simple like this:

client.Get() -> write cmd -> read response data.

@monkey92t, how about exposing an optional hook to allow users, who wish to have fully context-aware behaviour, to choose to implement the "set deadline on context end" in one of the multiple ways they see fit, e.g.:

1. With go 1.21+, as suggested above, using context.AfterFunc()
2. Using a separate routine to await context to complete
3. Some other approach

This should avoid having the authors/maintainers having to reason about increased complexity and/or reduced performance in the library. (E.g. like so, maybe in conjunction with pubsub context awareness)

[notrobpike]

@monkey92t I can definitely understand a desire to keep the API simpler, and performant. However this is a go-specific project, is it not? By not using the context deadlines you are going against the grain. This makes the calling code more complex, or at minimum, less idiomatic (goth) and ergo with more mental overhead for your users. The tradeoff is not a good one IMO. Esp when the API for BLMove() (eg) still takes a ctx arg but ignores it. I might go so far as to call this API hostile.

I'm not following along very closely, so excuse any misunderstandings on my part.