Confusion around returned error when lock not obtained due to timeout

[pranay1208]

Description

Per the example in the official documentation (see: https://pkg.go.dev/github.com/bsm/redislock@v0.9.4#example-Client.Obtain-CustomDeadline), if the Obtain function fails to obtain lock, it will return redislock.ErrNotObtained.

For the following code, lets assume the lock is already obtained and not released.

locker := redislock.New(client)

// Retry every 500ms, for up-to a minute
backoff := redislock.LinearBackoff(500 * time.Millisecond)
ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Minute))
defer cancel()

// Obtain lock with retry + custom deadline
lock, err := locker.Obtain(ctx, "my-key", time.Second, &redislock.Options{
	RetryStrategy: backoff,
})
if err == redislock.ErrNotObtained {
	fmt.Println("Could not obtain lock!")
} else if err != nil {
	log.Fatalln(err)
}

Expectation: console will print Could not obtain lock

Observed behaviour: console will print context deadline exceeded and halt function execution (due to log.Fatalln).

Why this occurs

Seems like this change was brought about in #63 and corresponding PR #64

select {
case <-ctx.Done():
	return nil, ctx.Err()
case <-ticker.C:
}

Suggestion

Replace return nil, ctx.Err() with return nil, errors.Join(ctx.Err(), ErrNotObtained)

This way, if someone does not care why the lock could not be obtained, then they can still check against the ErrNotObtained error.

// assume cannot acquire lock because context deadline is exceeded
lock, err := locker.Obtain(ctx, "key", time.Second, &redislock.Options{RetryStrategy: linearBackoff})

// this condition will be true
if errors.Is(err, redislock.ErrNotObtained) {
    fmt.Println("lock not obtained")
}

// this condition will also be true
if errors.Is(err, context.DeadlineExceeded) {
    fmt.Println("context deadline exceeded")
}

I'd be happy to make an MR to contribute this change if you think this is something valuable. Looking forward to your response, thanks

[pranay1208]

Alternately I would suggest updating the documentation here (https://pkg.go.dev/github.com/bsm/redislock@v0.9.4#section-readme) to compare against the appropriate error (i.e. context.DeadlineExceeded)

[philippgille]

I think it's reasonable that if an upstream defined timeout (from the passed context) is met, the error is "context deadline exceeded" and not "Could not obtain lock", but agree that a join could also be useful.

But I stumbled upon a slightly different case: When you as a caller have a basic context without timeout, then redislock sets a timeout on its own based on the TTL, which doesn't make sense to me.

Let's say I have 50 services in a distributed system, and I choose a short TTL of one second and a retry strategy that goes for 10 seconds. Now instead of retrying for 10 seconds, it only retries for one second. This is very unintuitive and hidden/implicit behavior.

The responsible code is here: https://github.com/bsm/redislock/blob/v0.9.4/redislock.go#L71-L76

It says // make sure we don't retry forever, but shouldn't that be up to the retry strategies instead?

Code to reproduce

package main

import (
	"context"
	"fmt"
	"log"
	"time"

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

func main() {
	client := redis.NewClient(&redis.Options{Network: "tcp", Addr: "127.0.0.1:6379"})
	defer client.Close()

	locker := redislock.New(client)

	// Simulate other processes in the distributed system holding the lock. Even
	// with 1s TTL, multiple other processes take turns holding the lock. We simulate
	// this by locking for 10 seconds.
	go func() {
		_, err := locker.Obtain(context.Background(), "my-key", 10*time.Second, nil)
		if err != nil {
			log.Println("Error in goroutine:", err)
		}
		time.Sleep(10 * time.Second)
	}()

	// Now on our side we want to obtain the lock too.

	// Retry every 500ms
	retry := redislock.LinearBackoff(500 * time.Millisecond)
	// But at most for 5 seconds overall, so 10 times
	retry = redislock.LimitRetry(retry, 10)

	// Try to obtain the lock with retry, so we expect this to retry for 5 seconds
	now := time.Now()
	_, err := locker.Obtain(context.Background(), "my-key", time.Second, &redislock.Options{
		RetryStrategy: retry,
	})
	fmt.Printf("Error after %v: %v\n", time.Since(now), err) // Error after 1.001695042s: context deadline exceeded
}

Prints Error after 1.001695042s: context deadline exceeded, so retrying only 1 second instead of 10