Fix the silently dropped fractional token #3433
Conversation
| long millisPassed = Math.subtractExact(t, lastTokenGenerationMillis); | ||
| lastTokenGenerationMillis = t; | ||
| tokens = Math.min(maxTokens, tokens + ((long) (millisPassed * tokensPerMilli))); | ||
| tokens = Math.min((double) maxTokens, tokens + (millisPassed * tokensPerMilli)); |
There was a problem hiding this comment.
I believe a more robust fix would be to count the elapsed time in longer periods rather than computing small fractional increments. Note that floating point addition may not increase the value if given sufficient difference in exponents.
shell> double x = 1e100;
x ==> 1.0E100
jshell> x + 0.0000001
$2 ==> 1.0E100
There was a problem hiding this comment.
That's a valid point, but only happen in an extreme case. For IEEE 754 double precision, the relative epsilon is ~2.2e-16. The issue would occur when: tokens + increment == tokens, which happens when increment < tokens * epsilon
For example:
- If maxTokens = 1e15 and tokensPerMilli = 0.001, the increment would be lost
- But for practical values like maxTokens = 10000, adding 0.001 will always register
The Java long can reach up to 1e13, which is at the edge of losing the increment. Mostly likely it won't happen, users may just disable the rate limiter in that case.
There was a problem hiding this comment.
I have changed it to use long only, please take another look.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
|
||
| tokens = maxTokens; | ||
| lastTokenGenerationMillis = instantSource.millis(); | ||
| milliTokens = maxMilliTokens; |
There was a problem hiding this comment.
nit: why not this.*** as with the above assignments?
| // After 900ms total, we should have 0.9 tokens - not enough yet | ||
| // Add 100ms more to reach exactly 1 token | ||
| clock.add(Duration.ofMillis(100)); | ||
| Assertions.assertTrue(tokenBucket.tryAcquire()); |
There was a problem hiding this comment.
nit: why not assertCanAcquire(tokenBucket, 1) as above?
| long grant = millisPassed * tokensPerSecond; | ||
| if (tokensPerSecond != 0 && grant / tokensPerSecond != millisPassed) { | ||
| // Overflow occurred. If this much time passed, the bucket would be full anyway. | ||
| milliTokens = maxMilliTokens; |
There was a problem hiding this comment.
This case is not tested by TokenBucketTest. Is it necessary?
| long millisPassed = Math.subtractExact(t, lastUpdateMillis); | ||
| lastUpdateMillis = t; |
There was a problem hiding this comment.
IMHO, it would be more robust (and easier to understand) to advance lastUpdateMillis only when millisPassed is large enough to grant at least one token. We'll move lastUpdateMillis only to the point that exactly matches the boundary that results in that exact grant. This way we can count exact tokens (not milli tokens), which increases the operational range of token values (without overflow).
2 participants
The old implementation silently dropped fractional token accrual. That means any rate below 1000 tokens/sec could never refill the bucket: each call adds <1 token, cast to long becomes 0, so tokens stay at 0 forever. Switching to double preserves fractional accumulation so the bucket refills correctly over time.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)