From 6731defd0291a817878b1d5186a7c279f64176a5 Mon Sep 17 00:00:00 2001 From: Rueian Date: Sun, 25 Aug 2024 12:11:16 +0800 Subject: [PATCH] feat: improve error messages of rueidislock Signed-off-by: Rueian --- rueidislock/lock.go | 42 ++++++++++++++++++++++++---------------- rueidislock/lock_test.go | 10 +++++----- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/rueidislock/lock.go b/rueidislock/lock.go index 44343500..a9d90ce0 100644 --- a/rueidislock/lock.go +++ b/rueidislock/lock.go @@ -3,6 +3,7 @@ package rueidislock import ( "context" "errors" + "fmt" "strconv" "strings" "sync" @@ -168,7 +169,7 @@ func (m *locker) acquire(ctx context.Context, key, val string, deadline time.Tim } cancel() if err = resp.Error(); rueidis.IsRedisNil(err) { - return ErrNotLocked + return fmt.Errorf("%w: key %s is held by others", ErrNotLocked, key) } return err } @@ -286,7 +287,7 @@ func (m *locker) onInvalidations(messages []rueidis.RedisMessage) { } } -func (m *locker) try(ctx context.Context, cancel context.CancelFunc, name string, g *gate, force bool) context.CancelFunc { +func (m *locker) try(ctx context.Context, cancel context.CancelFunc, name string, g *gate, force bool) (context.CancelFunc, error) { var err error val := random() @@ -354,7 +355,7 @@ func (m *locker) try(ctx context.Context, cancel context.CancelFunc, name string case <-ch: default: } - if err != ErrNotLocked { + if !errors.Is(err, ErrNotLocked) { if err = m.acquire(ctx, key, val, deadline, force); force && err == nil { m.mu.RLock() if m.gates != nil { @@ -389,16 +390,20 @@ func (m *locker) try(ctx context.Context, cancel context.CancelFunc, name string return func() { cancel() <-done - } + }, nil } <-done - return nil + if err == nil { + err = fmt.Errorf("%w: failed to acquire the majority of keys (%d/%d)", ErrNotLocked, atomic.LoadInt32(&acquired), m.totalcnt) + } + return cancel, err } func (m *locker) ForceWithContext(ctx context.Context, name string) (context.Context, context.CancelFunc, error) { + var err error ctx, cancel := context.WithCancel(ctx) if g := m.forcegate(name); g != nil { - if cancel := m.try(ctx, cancel, name, g, true); cancel != nil { + if cancel, err = m.try(ctx, cancel, name, g, true); err == nil { return ctx, cancel, nil } m.mu.Lock() @@ -410,13 +415,17 @@ func (m *locker) ForceWithContext(ctx context.Context, name string) (context.Con m.mu.Unlock() } cancel() - return ctx, cancel, ErrNotLocked + if err == nil { + err = ErrLockerClosed + } + return ctx, cancel, err } func (m *locker) TryWithContext(ctx context.Context, name string) (context.Context, context.CancelFunc, error) { + var err error ctx, cancel := context.WithCancel(ctx) if g := m.trygate(name); g != nil { - if cancel := m.try(ctx, cancel, name, g, false); cancel != nil { + if cancel, err = m.try(ctx, cancel, name, g, false); err == nil { return ctx, cancel, nil } m.mu.Lock() @@ -428,23 +437,22 @@ func (m *locker) TryWithContext(ctx context.Context, name string) (context.Conte m.mu.Unlock() } cancel() - return ctx, cancel, ErrNotLocked + if err == nil { + err = fmt.Errorf("%w: the lock is held by others or the locker is closed", ErrNotLocked) + } + return ctx, cancel, err } -func (m *locker) WithContext(ctx context.Context, name string) (context.Context, context.CancelFunc, error) { +func (m *locker) WithContext(src context.Context, name string) (context.Context, context.CancelFunc, error) { for { - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := context.WithCancel(src) g, err := m.waitgate(ctx, name) if g != nil { - if cancel := m.try(ctx, cancel, name, g, false); cancel != nil { + if cancel, err := m.try(ctx, cancel, name, g, false); err == nil { return ctx, cancel, nil } m.mu.Lock() - if g.w--; g.w == 0 && err != nil { // delete g from m.gates only when exiting with an error. - if m.gates[name] == g { - delete(m.gates, name) - } - } + g.w-- // do not delete g from m.gates here. m.mu.Unlock() } if cancel(); err != nil { diff --git a/rueidislock/lock_test.go b/rueidislock/lock_test.go index cdfb5cf5..df473b67 100644 --- a/rueidislock/lock_test.go +++ b/rueidislock/lock_test.go @@ -457,7 +457,7 @@ func TestLocker_TryWithContext(t *testing.T) { if err != nil { t.Fatal(err) } - if _, _, err := locker.TryWithContext(ctx, lck); err != ErrNotLocked { + if _, _, err := locker.TryWithContext(ctx, lck); !errors.Is(err, ErrNotLocked) { t.Fatal(err) } cancel() @@ -486,7 +486,7 @@ func TestLocker_ForceWithContextThenTryWithContext(t *testing.T) { if err != nil { t.Fatal(err) } - if _, _, err := locker.TryWithContext(ctx, lck); err != ErrNotLocked { + if _, _, err := locker.TryWithContext(ctx, lck); !errors.Is(err, ErrNotLocked) { t.Fatal(err) } cancel() @@ -528,7 +528,7 @@ func TestLocker_TryWithContext_MultipleLocker(t *testing.T) { for j := 0; j < cnt; j++ { for { _, cancel, err := l.TryWithContext(ctx, lck) - if err != nil && err != ErrNotLocked { + if err != nil && !errors.Is(err, ErrNotLocked) { t.Error(err) return } @@ -663,7 +663,7 @@ func TestLocker_Close(t *testing.T) { wg.Add(10) for i := 0; i < 10; i++ { go func() { - if _, _, err := locker.WithContext(context.Background(), lck); err != ErrLockerClosed { + if _, _, err := locker.WithContext(context.Background(), lck); !errors.Is(err, ErrLockerClosed) { t.Error(err) } wg.Done() @@ -676,7 +676,7 @@ func TestLocker_Close(t *testing.T) { if err := ctx.Err(); !errors.Is(err, context.Canceled) { t.Fatal(err) } - if _, _, err := locker.WithContext(context.Background(), lck); err != ErrLockerClosed { + if _, _, err := locker.WithContext(context.Background(), lck); !errors.Is(err, ErrLockerClosed) { t.Fatal(err) } }