Fix race when initializing ringBufferRateLimiter #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This closes mholt#36. Fixes a race condition between ringBufferRateLimiter creation and its insertion into a map. Do this by locking the entire map when we get or insert a ringBufferRateLimiter. I have replaced use of sync.Map with a normal `map[string]*ringBufferRateLimiter` and a `sync.Mutex`. They are passed around with a `rateLimitersMap` struct. I've factored out logic into methods of rateLimitersMap, which enables some careful use of defer `rlm.mu.Unlock()`` to avoid leaving a lock held open on `panic()`. We didn't see a need for a sync.Map. The docs suggest against using it for type safety, and none of the suggested use cases apply. https://pkg.go.dev/sync#Map. Let me know if I'm misunderstanding the use case (very possible!). I've removed the sync.Pool, for now. Since ringBufferRateLimiter creation and insertion is fully synchronized, I didn't see a need for it. Note that some of the defensive refactoring is not strictly required--I have a change that preserves the existing data structures, but I think the suggested changeset is an overall improvement in maintainability. 65ad951. Some discussion of the performance impact and profiles is here #1. TL;DR, no meaningful impact to CPU, memory, or latency. This implementation could be optimized by replacing the normal mutex with a RWMutex, but it would be a marginal improvement (if any) in exchange for much more complicated locking semantics.
|
Hi, wow!! Thank you for this. Sorry for the lateness, I was out this weekend with the family. I'll look at this soon! |
|
How have your instances been this last week, @inahga? I'll sleep better once this is merged 🙂 |
mholt
approved these changes
Mar 13, 2024
I'll let you know by Monday, I was slow to deploy it this week. |
|
@DzigaV We've run this in production for a week now, with no signs of leakage. Feeling pretty confident this fixed it. |
|
That's great news!! Thank you for the contribution and the work required to get it figured out. Glad it's working 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This closes #36.
Fixes a race condition between ringBufferRateLimiter creation and its insertion into a map. Do this by locking the entire map when we get or insert a ringBufferRateLimiter.
I have replaced use of sync.Map with a normal
map[string]*ringBufferRateLimiterand async.Mutex. They are passed around with arateLimitersMapstruct. I've factored out logic into methods ofrateLimitersMap, which enables some careful use of deferrlm.mu.Unlock()to avoid leaving a lock held open onpanic().We didn't see a need for a sync.Map. The docs suggest against using it for type safety, and none of the suggested use cases apply. https://pkg.go.dev/sync#Map. Let me know if I'm misunderstanding the use case (very possible!).
I've removed the sync.Pool, for now. Since ringBufferRateLimiter creation and insertion is fully synchronized, I didn't see a need for it.
Note that some of the defensive refactoring is not strictly required--I have a change that preserves the existing data structures, but I think the suggested changeset is an overall improvement in maintainability. divviup@65ad951.
Some discussion of the performance impact and profiles is here divviup#1. TL;DR, no meaningful impact to CPU, memory, or latency. This implementation could be optimized by replacing the normal mutex with a RWMutex, but it would be a marginal improvement (if any) in exchange for much more complicated locking semantics.
I suggest waiting before merging this, while we let it sit in production for a while longer, to ensure the problem is fully fixed. I'll let it sit for about a week. In the mean time, this is ready for code review.