-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🩹 Fix: Memory leak removal in the idempotency middleware #3263
🩹 Fix: Memory leak removal in the idempotency middleware #3263
Conversation
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes modify the Changes
Assessment against linked issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
middleware/idempotency/locker.go (2)
13-16
: Consider using an atomic integer forlocked
to reinforce thread safety.While this approach is acceptable under the current locking scheme, using an atomic integer (e.g.,
atomic.AddInt32
) forlocked
might eliminate any risk of data races if the usage pattern changes in the future.
62-62
: Optional: specify a default map capacity or document capacity assumptions.When creating the map, consider passing an estimated capacity if usage patterns anticipate a high volume of concurrent keys. This micro-optimization may help reduce rehashing overhead. Otherwise, documenting approximate usage expectations could suffice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/idempotency/locker.go
(1 hunks)
🔇 Additional comments (2)
middleware/idempotency/locker.go (2)
25-30
: Initialization logic for countedLock
looks correct.
The code correctly initializes a new countedLock
as needed and increments its counter. This helps avoid memory leaks by ensuring that locks are properly created and tracked in the map.
47-55
: Ensure concurrency remains consistent after map deletion.
Currently, the key is removed from the map (line 51) before unlocking the countedLock
(line 55). If a new goroutine tries to lock the same key in the short interval between the delete
call and lock.mu.Unlock()
, it might create a new countedLock
while the old one is still locked. This could be acceptable if ephemeral locks are desired. However, if strict synchronization is a requirement (e.g., not allowing reuse of a key until the prior lock is fully released), consider reversing the unlock and map deletion steps or clarifying this behavior in documentation.
@sunnyyssh Unit-tests are failing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesnt make sense?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3263 +/- ##
==========================================
- Coverage 84.40% 84.35% -0.06%
==========================================
Files 116 116
Lines 11485 11497 +12
==========================================
+ Hits 9694 9698 +4
- Misses 1374 1380 +6
- Partials 417 419 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
b8d263d
to
4dd645b
Compare
@sunnyyssh Can you add a benchmark in |
The main idea is that one idempotency key wouldn't be used once again frequently just because the operation completes successfully in most cases. So, trying to benchmark the performance of "key-deleting" MemoryLock with frequently repeated keys, we will have not real-world results. So, I tried to make benchmarks checking new and old implementations and got interesting result. 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)
}
}
} Old implementation has shown extremely inefficient result Trying to figure out why old implementation has so bad performance and memory allocations I used pprof and got this: map is inefficient for storing big amounts of data because it always re-allocates its buckets when resizing. Also I've written parallel benchmarks that check unique keys and repeated 3 times keys. There is also big difference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/idempotency/locker_test.go
(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
middleware/idempotency/locker_test.go
100-100: Error return value of lock.Lock
is not checked
(errcheck)
101-101: Error return value of lock.Unlock
is not checked
(errcheck)
114-114: Error return value of lock.Lock
is not checked
(errcheck)
115-115: Error return value of lock.Unlock
is not checked
(errcheck)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
middleware/idempotency/locker_test.go (3)
65-84
: Consider adding test cases with different key sizes.The benchmark effectively tests the basic lock/unlock operations. To better understand memory behavior, consider adding test cases with varying key sizes to measure the impact on memory allocation and performance.
Example addition:
func Benchmark_MemoryLock_KeySizes(b *testing.B) { sizes := []int{8, 64, 512, 1024} // bytes for _, size := range sizes { b.Run(fmt.Sprintf("KeySize_%d", size), func(b *testing.B) { keys := make([]string, b.N) padding := strings.Repeat("x", size-8) // -8 for base number for i := range keys { keys[i] = fmt.Sprintf("%d%s", i, padding) } // ... rest of the benchmark }) } }
88-91
: Consider making the key pool size configurable.The hard-coded pool size of 1,000,000 keys might be excessive for some test environments. Consider making it configurable or scaling it based on
b.N
.const defaultKeyPoolSize = 1_000_000 func getKeyPoolSize(b *testing.B) int { if b.N < defaultKeyPoolSize { return b.N } return defaultKeyPoolSize }
115-116
: Improve the comment clarity for the repeated keys logic.The current comment could be clearer about how the division by 3 works.
- // Division by 3 ensures that index will be repreated exactly 3 times + // We increment the counter by 1 each time but divide by 3, + // so each index will be used for 3 consecutive increments. + // Example: counter values 0,1,2 -> index 0, values 3,4,5 -> index 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/idempotency/locker_test.go
(2 hunks)
🔇 Additional comments (2)
middleware/idempotency/locker_test.go (2)
4-5
: LGTM: Appropriate imports added for benchmarking.
The new imports support the benchmark functionality - strconv
for key generation and sync/atomic
for thread-safe operations.
86-127
: Well-designed benchmarks for memory leak verification.
The parallel benchmarks effectively test the memory leak fix by:
- Testing unique keys to establish baseline performance
- Testing repeated keys to verify proper cleanup of unused locks
- Using parallel execution to ensure thread safety
The combination of these scenarios provides good coverage for verifying the memory leak fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
The MemoryLock in locker.go doesn't let GC collect unused mutexes.
I propose key removal from map to let GC collect unused Mutex.
Fixes #3262
Changes introduced
I replaced just
sync.Mutex
withIt will count the difference between Lock and Unlock and if it equals 0 then the key can be removed from
keys
map in the MemoryLock's own mutex lock scope.So, now GC is happy to collect 😊