Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a Registry component that manages per-user rate limiters, enabling independent rate limiting for multiple users. The implementation uses a thread-safe map that lazily creates Limiter instances for each user identifier.
Key changes:
- New
Registrytype with concurrent-safe limiter management - Comprehensive test suite including concurrency tests
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| token/registry.go | Implements Registry type with NewRegistry constructor and Allow method for managing per-user rate limiters |
| token/registry_test.go | Adds test suite covering basic functionality, edge cases, and concurrent access patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "fmt" | ||
| "sync" | ||
| ) | ||
|
|
There was a problem hiding this comment.
The Identifier type lacks documentation. Public types should have documentation comments explaining their purpose and usage.
| // Identifier represents a unique key used to look up or create a Limiter in a Registry. |
|
|
||
| var wg sync.WaitGroup | ||
|
|
||
| // Create 100 different users concurrently |
There was a problem hiding this comment.
The comment is misleading - it states "Create 100 different users concurrently" but the actual implementation only creates 26 unique users due to the modulo operation on line 108. Update the comment to accurately reflect the test behavior.
| // Create 100 different users concurrently | |
| // Issue 100 concurrent requests across up to 26 different users |
| for _, user := range users { | ||
| limiter, err := NewLimiter(capacity, rate) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fail to create a new limiter %v", err) |
There was a problem hiding this comment.
The error message should be grammatically correct and follow Go conventions. Change "fail to create a new limiter" to "failed to create limiter" and use proper error wrapping format with a colon.
| return nil, fmt.Errorf("fail to create a new limiter %v", err) | |
| return nil, fmt.Errorf("failed to create limiter: %w", err) |
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
| lim, ok := r.limiters[key] | ||
| if !ok { |
There was a problem hiding this comment.
The error returned by NewLimiter is silently ignored. While this should never fail given the capacity and rate have been validated in NewRegistry, this could hide bugs if the Limiter implementation changes. Consider handling the error or at least adding a comment explaining why it's safe to ignore.
| if !ok { | |
| if !ok { | |
| // NewLimiter is expected not to fail here because r.capacity and r.rate | |
| // were validated when the Registry was created in NewRegistry. | |
| // If NewLimiter's contract changes to allow errors for these values, | |
| // this call site should be updated to handle them explicitly. |
| wg.Add(1) | ||
| go func(id int) { | ||
| defer wg.Done() | ||
| user := Identifier(string(rune('a' + id%26))) |
There was a problem hiding this comment.
The user generation logic only creates 26 unique users (characters 'a' through 'z'), not 100 as the comment suggests. The expression 'a' + id%26 will cycle through the same 26 characters. This reduces the effectiveness of the concurrent test since many goroutines will be contending for the same user identifiers. Consider using a unique identifier for each goroutine, such as formatting the id directly into a string.
| type Registry struct { | ||
| mu sync.Mutex | ||
| limiters map[Identifier]*Limiter | ||
| capacity, rate float64 | ||
| } |
There was a problem hiding this comment.
The Registry type and its methods lack documentation comments. Public types and their exported methods should have documentation comments that start with the type/function name, following Go documentation conventions.
| limiters map[Identifier]*Limiter | ||
| capacity, rate float64 | ||
| } | ||
|
|
There was a problem hiding this comment.
The NewRegistry function lacks documentation. It should have a comment describing what it does, explaining its parameters (capacity, rate, and optional users), and documenting when it returns an error.
| // NewRegistry creates a Registry that manages token limiters configured with the | |
| // provided capacity and rate. For each optional user identifier supplied in | |
| // users, a corresponding Limiter is created and stored in the registry. | |
| // | |
| // capacity specifies the maximum number of tokens that each limiter can hold, | |
| // and rate specifies the rate at which tokens are replenished. An error is | |
| // returned if creating any underlying Limiter fails. |
| rate: rate, | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The Allow method lacks documentation. It should have a comment explaining its behavior, particularly that it creates a new limiter for unknown identifiers and returns whether the request is allowed.
| // Allow reports whether a request for the given identifier is permitted, | |
| // creating a new limiter with the registry's capacity and rate if none exists. |
No description provided.