-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix AttemptAcquire(0) when token isn't available #123841
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @agocke, @VSadov |
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.
Pull request overview
Fixes TokenBucketRateLimiter behavior where AttemptAcquire(0)/AcquireAsync(0) could succeed with only fractional tokens available (inconsistent with CurrentAvailablePermits truncation), and adds tests covering the fractional-token scenarios.
Changes:
- Update zero-token acquisition fast paths to require at least one whole token (
_tokenCount >= 1) rather than any positive fractional value. - Ensure queued zero-token requests are only fulfilled once at least one whole token is available.
- Add unit tests reproducing and validating the fractional-token behavior for both sync and async zero-token acquisition.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs | Tightens zero-token acquisition/queue fulfillment conditions to treat fractional _tokenCount as unavailable. |
| src/libraries/System.Threading.RateLimiting/tests/TokenBucketRateLimiterTests.cs | Adds targeted tests to cover fractional-token edge cases for AttemptAcquire(0) and AcquireAsync(0). |
| var acquireTask = limiter.AcquireAsync(0); | ||
| Assert.False(acquireTask.IsCompleted); | ||
|
|
||
| Replenish(limiter, 1); | ||
| using var lease2 = await acquireTask; | ||
| Assert.True(lease2.IsAcquired); | ||
| } |
Copilot
AI
Jan 31, 2026
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.
In this test, awaiting acquireTask without any timeout can hang the test run if the replenishment math/rounding doesn’t quite reach the >= 1 threshold (especially since the test intentionally works with fractional tokens). Consider awaiting with a bounded timeout (e.g., convert to Task and use WaitAsync/WhenAny) and/or replenishing with a larger elapsed time on the second replenish to guarantee completion.
| Replenish(limiter, 1); | ||
| Assert.Equal(1, limiter.GetStatistics()!.CurrentAvailablePermits); |
Copilot
AI
Jan 31, 2026
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 assertion/await sequence is sensitive to timing conversion and truncation: if the final replenishment leaves _tokenCount just under 1.0, CurrentAvailablePermits will still be 0 and the awaited task may not complete. To make the test robust, consider replenishing with a slightly larger elapsed time before asserting/awaiting (or assert availability using a threshold rather than relying on hitting exactly 1.0).
| Replenish(limiter, 1); | |
| Assert.Equal(1, limiter.GetStatistics()!.CurrentAvailablePermits); | |
| Replenish(limiter, 2); | |
| Assert.True(limiter.GetStatistics()!.CurrentAvailablePermits >= 1); |
| else if (_tokenCount >= nextPendingRequest.Count && (nextPendingRequest.Count > 0 || _tokenCount >= 1)) | ||
| { |
Copilot
AI
Jan 31, 2026
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 fulfillment condition works, but it’s hard to read due to redundant comparisons (for Count > 0, _tokenCount >= Count already implies >= 1). Consider rewriting it in a more direct form (e.g., branch on nextPendingRequest.Count == 0) to reduce cognitive load and help avoid future mistakes around the tokenCount == 0 special-case.
Fixes #118192
Updated the
_tokenCountchecks to be fraction-aware (i.e. no longer >0, but >=1) and added unit tests to reproduce the issue.