From e9fef500b70355d9e3ed53079a230a8849891e29 Mon Sep 17 00:00:00 2001 From: sunnyyssh Date: Wed, 25 Dec 2024 13:16:20 +0300 Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=A9=B9=20Fix:=20Add=20key=20removal?= =?UTF-8?q?=20in=20MemoryLock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- middleware/idempotency/locker.go | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/middleware/idempotency/locker.go b/middleware/idempotency/locker.go index 2c3348b8f3..c3417dd6ca 100644 --- a/middleware/idempotency/locker.go +++ b/middleware/idempotency/locker.go @@ -10,42 +10,56 @@ type Locker interface { Unlock(key string) error } +type countedLock struct { + mu sync.Mutex + locked int +} + type MemoryLock struct { - keys map[string]*sync.Mutex + keys map[string]*countedLock mu sync.Mutex } func (l *MemoryLock) Lock(key string) error { l.mu.Lock() - mu, ok := l.keys[key] + lock, ok := l.keys[key] if !ok { - mu = new(sync.Mutex) - l.keys[key] = mu + lock = new(countedLock) + l.keys[key] = lock } + lock.locked++ l.mu.Unlock() - mu.Lock() + lock.mu.Lock() return nil } func (l *MemoryLock) Unlock(key string) error { l.mu.Lock() - mu, ok := l.keys[key] - l.mu.Unlock() + lock, ok := l.keys[key] if !ok { // This happens if we try to unlock an unknown key + l.mu.Unlock() return nil } - mu.Unlock() + lock.locked-- + if lock.locked <= 0 { + // This happens if countedLock is used to Lock and Unlock the same number of times + // So, we can delete the key to prevent memory leak + delete(l.keys, key) + } + l.mu.Unlock() + + lock.mu.Unlock() return nil } func NewMemoryLock() *MemoryLock { return &MemoryLock{ - keys: make(map[string]*sync.Mutex), + keys: make(map[string]*countedLock), } } From d68143afcc23b3b59011b667c7c37362d5889e5b Mon Sep 17 00:00:00 2001 From: sunnyyssh Date: Wed, 25 Dec 2024 13:56:07 +0300 Subject: [PATCH 2/6] Fixed concurrent deletion. --- middleware/idempotency/locker.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/middleware/idempotency/locker.go b/middleware/idempotency/locker.go index c3417dd6ca..187179b12b 100644 --- a/middleware/idempotency/locker.go +++ b/middleware/idempotency/locker.go @@ -32,6 +32,14 @@ func (l *MemoryLock) Lock(key string) error { lock.mu.Lock() + lock.locked-- + if lock.locked <= 0 { + // This happens if countedLock is used to Lock and Unlock the same number of times + // So, we can delete the key to prevent memory leak + delete(l.keys, key) + } + l.mu.Unlock() + return nil } @@ -44,14 +52,6 @@ func (l *MemoryLock) Unlock(key string) error { return nil } - lock.locked-- - if lock.locked <= 0 { - // This happens if countedLock is used to Lock and Unlock the same number of times - // So, we can delete the key to prevent memory leak - delete(l.keys, key) - } - l.mu.Unlock() - lock.mu.Unlock() return nil From 3a5c54aacb1a5b2c0ec6e45154f173638e9ee81a Mon Sep 17 00:00:00 2001 From: sunnyyssh Date: Wed, 25 Dec 2024 19:35:12 +0300 Subject: [PATCH 3/6] Fix: idempotency middleware's MemoryLock --- middleware/idempotency/locker.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/middleware/idempotency/locker.go b/middleware/idempotency/locker.go index 187179b12b..f24db382a5 100644 --- a/middleware/idempotency/locker.go +++ b/middleware/idempotency/locker.go @@ -32,14 +32,6 @@ func (l *MemoryLock) Lock(key string) error { lock.mu.Lock() - lock.locked-- - if lock.locked <= 0 { - // This happens if countedLock is used to Lock and Unlock the same number of times - // So, we can delete the key to prevent memory leak - delete(l.keys, key) - } - l.mu.Unlock() - return nil } @@ -51,9 +43,19 @@ func (l *MemoryLock) Unlock(key string) error { l.mu.Unlock() return nil } + l.mu.Unlock() lock.mu.Unlock() + l.mu.Lock() + lock.locked-- + if lock.locked <= 0 { + // This happens if countedLock is used to Lock and Unlock the same number of times + // So, we can delete the key to prevent memory leak + delete(l.keys, key) + } + l.mu.Unlock() + return nil } From 98d7ae9cf85215326d32df79b7b1d340290add24 Mon Sep 17 00:00:00 2001 From: sunnyyssh Date: Wed, 25 Dec 2024 22:52:32 +0300 Subject: [PATCH 4/6] Add MemoryLock benchmarks. --- middleware/idempotency/locker_test.go | 58 +++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/middleware/idempotency/locker_test.go b/middleware/idempotency/locker_test.go index 3b4a3ca78a..4a0f20217a 100644 --- a/middleware/idempotency/locker_test.go +++ b/middleware/idempotency/locker_test.go @@ -1,6 +1,8 @@ package idempotency_test import ( + "strconv" + "sync/atomic" "testing" "time" @@ -59,3 +61,59 @@ func Test_MemoryLock(t *testing.T) { require.NoError(t, err) } } + +func Benchmark_MemoryLock(b *testing.B) { + keys := make([]string, b.N) + for i := range keys { + keys[i] = strconv.Itoa(i) + } + + lock := idempotency.NewMemoryLock() + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + key := keys[i] + if err := lock.Lock(key); err != nil { + b.Fatal(err) + } + if err := lock.Unlock(key); err != nil { + b.Fatal(err) + } + } +} + +func Benchmark_MemoryLock_Parallel(b *testing.B) { + // In order to prevent using repeated keys I pre-allocate keys + keys := make([]string, 1_000_000) + for i := range keys { + keys[i] = strconv.Itoa(i) + } + + b.Run("UniqueKeys", func(b *testing.B) { + lock := idempotency.NewMemoryLock() + var keyI atomic.Int32 + b.RunParallel(func(p *testing.PB) { + for p.Next() { + i := int(keyI.Add(1)) % len(keys) + key := keys[i] + lock.Lock(key) + lock.Unlock(key) + } + }) + }) + + b.Run("Repeated3TimesKeys", func(b *testing.B) { + lock := idempotency.NewMemoryLock() + var keyI atomic.Int32 + b.RunParallel(func(p *testing.PB) { + for p.Next() { + // Division by 3 ensures that index will be repreated exactly 3 times + i := int(keyI.Add(1)) / 3 % len(keys) + key := keys[i] + lock.Lock(key) + lock.Unlock(key) + } + }) + }) +} From d7653317afea0f45fdf190c1ba1372b74c7514c8 Mon Sep 17 00:00:00 2001 From: sunnyyssh Date: Wed, 25 Dec 2024 23:00:45 +0300 Subject: [PATCH 5/6] Updated benchmarks: Add returning error handling --- middleware/idempotency/locker_test.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/middleware/idempotency/locker_test.go b/middleware/idempotency/locker_test.go index 4a0f20217a..3073f007df 100644 --- a/middleware/idempotency/locker_test.go +++ b/middleware/idempotency/locker_test.go @@ -97,8 +97,12 @@ func Benchmark_MemoryLock_Parallel(b *testing.B) { for p.Next() { i := int(keyI.Add(1)) % len(keys) key := keys[i] - lock.Lock(key) - lock.Unlock(key) + if err := lock.Lock(key); err != nil { + b.Fatal(err) + } + if err := lock.Unlock(key); err != nil { + b.Fatal(err) + } } }) }) @@ -111,8 +115,12 @@ func Benchmark_MemoryLock_Parallel(b *testing.B) { // Division by 3 ensures that index will be repreated exactly 3 times i := int(keyI.Add(1)) / 3 % len(keys) key := keys[i] - lock.Lock(key) - lock.Unlock(key) + if err := lock.Lock(key); err != nil { + b.Fatal(err) + } + if err := lock.Unlock(key); err != nil { + b.Fatal(err) + } } }) }) From 84ba8eb6918d56a28346bc6cda9cbeb9589a732d Mon Sep 17 00:00:00 2001 From: sunnyyssh Date: Wed, 25 Dec 2024 23:49:26 +0300 Subject: [PATCH 6/6] Renamed benchmark: RepeatedKeys --- middleware/idempotency/locker_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/middleware/idempotency/locker_test.go b/middleware/idempotency/locker_test.go index 3073f007df..81da15d3bf 100644 --- a/middleware/idempotency/locker_test.go +++ b/middleware/idempotency/locker_test.go @@ -107,7 +107,7 @@ func Benchmark_MemoryLock_Parallel(b *testing.B) { }) }) - b.Run("Repeated3TimesKeys", func(b *testing.B) { + b.Run("RepeatedKeys", func(b *testing.B) { lock := idempotency.NewMemoryLock() var keyI atomic.Int32 b.RunParallel(func(p *testing.PB) {