diff --git a/state/redis/README.md b/state/redis/README.md index 6729c2b..a62a308 100644 --- a/state/redis/README.md +++ b/state/redis/README.md @@ -69,14 +69,10 @@ with its own `go.mod` [go.mod:module]. `redis.go` imports the Go methods return `false` [redis.go:extendScript] [redis.go:releaseScript] [redis.go:State.ExtendLock] [redis.go:State.ReleaseLock]. -- `ExtendLock` passes `ttl.Milliseconds()` to the script, so it truncates - sub-millisecond precision [redis.go:State.ExtendLock]. -- A positive `ttl` under 1 ms therefore reaches the script as `0`. Redis - deletes a key given a non-positive `PEXPIRE` and returns `1`, so - `ExtendLock` deletes the lock and still returns `true` - [redis.go:State.ExtendLock] [redis.go:extendScript]. The runtime renews - with `ThreadLockTTL` (default 2 minutes), so only a sub-millisecond - configuration reaches this path. +- `ExtendLock` passes `ttl` to the script in whole milliseconds, so it + truncates sub-millisecond precision. A positive `ttl` under 1 ms becomes + 1 ms, as go-redis does for `SetNX`, so `PEXPIRE` never gets `0` and never + deletes the lock [redis.go:State.ExtendLock] [redis.go:extendScript]. - Redis errors from the state methods are returned unwrapped. `New` wraps a ping error as `"redis state: ping: %w"` [redis.go:New]. - `Shutdown` closes the client at most once. It returns the `Close` error from @@ -97,7 +93,7 @@ with its own `go.mod` [go.mod:module]. `redis.go` imports with the token and the caller's `ttl` [redis.go:State.AcquireLock] [redis.go:newToken]. - `ExtendLock` sends `EVAL extendScript` on `:lock:` with - `ARGV = [token, ttl in ms]` [redis.go:State.ExtendLock]. + `ARGV = [token, max(ttl in ms, 1)]` [redis.go:State.ExtendLock]. - `ReleaseLock` sends `EVAL releaseScript` on `:lock:` with `ARGV = [token]` [redis.go:State.ReleaseLock]. - `Shutdown` calls `Close` on the caller-supplied `redis.UniversalClient` diff --git a/state/redis/redis.go b/state/redis/redis.go index 9067b98..b776f51 100644 --- a/state/redis/redis.go +++ b/state/redis/redis.go @@ -109,7 +109,7 @@ func (s *State) ExtendLock(ctx context.Context, lease chat.LockLease, ttl time.D if ttl <= 0 { return false, errors.New("redis state: lock ttl must be positive") } - result, err := s.client.Eval(ctx, extendScript, []string{s.key("lock", lease.Key)}, lease.Token, ttl.Milliseconds()).Int() + result, err := s.client.Eval(ctx, extendScript, []string{s.key("lock", lease.Key)}, lease.Token, max(ttl.Milliseconds(), 1)).Int() if err != nil { return false, err } diff --git a/state/redis/redis_test.go b/state/redis/redis_test.go index 5a5cef3..5c141d2 100644 --- a/state/redis/redis_test.go +++ b/state/redis/redis_test.go @@ -38,6 +38,39 @@ func TestStateConformance(t *testing.T) { }) } +func TestExtendLockSubMillisecondTTLKeepsLock(t *testing.T) { + server := miniredis.RunT(t) + client := goredis.NewClient(&goredis.Options{Addr: server.Addr()}) + t.Cleanup(func() { + _ = client.Close() + }) + ctx := context.Background() + state, err := chatredis.New(ctx, chatredis.Options{Client: client, Prefix: "test"}) + if err != nil { + t.Fatalf("new redis state: %v", err) + } + + for _, ttl := range []time.Duration{time.Nanosecond, 500 * time.Microsecond} { + t.Run(ttl.String(), func(t *testing.T) { + key := "thread-" + ttl.String() + lease, acquired, err := state.AcquireLock(ctx, key, time.Minute) + if err != nil || !acquired { + t.Fatalf("AcquireLock = %v, %v; want true, nil", acquired, err) + } + extended, err := state.ExtendLock(ctx, lease, ttl) + if err != nil || !extended { + t.Fatalf("ExtendLock = %v, %v; want true, nil", extended, err) + } + if !server.Exists("test:lock:" + key) { + t.Fatal("lock key was deleted by ExtendLock") + } + if _, acquired, err := state.AcquireLock(ctx, key, time.Minute); err != nil || acquired { + t.Fatalf("second AcquireLock = %v, %v; want false, nil", acquired, err) + } + }) + } +} + func TestStateConformanceWithRedisContainer(t *testing.T) { testcontainers.SkipIfProviderIsNotHealthy(t)