Repository navigation
Fix RedissonRateLimiter discards permits returned by release() when it recalculates the currently available permits - #7343
Merged
mrniko merged 5 commits intoSep 16, 2026
Conversation
…it recalculates the currently available permits When the number of expired permits plus the currently available permits exceeds the rate, `tryAcquireAsync()` recalculates the available permits as `rate - used`, where `used` is the number of permits still in use. This discards permits returned by `release()`: a released permit is immediately added back to the available permits, but it is never reflected in `used`, so the recalculation overwrites and loses it. Skip the `rate - used` recalculation when `used + currentValue` already exceeds the rate, so that permits returned by `release()` are preserved instead of being overwritten. Added a regression test that acquires a permit via `tryAcquire()` while `used + available` exceeds the rate and verifies that the available permits are not recalculated to `rate - used`. The test fails on the current master branch and passes with this change. Signed-off-by: likerhythm <[email protected]>
mrniko
requested changes
Sep 15, 2026
| + "used = used + permits;" | ||
| + "end; " | ||
| + "currentValue = tonumber(rate) - used; " | ||
| + "if used + tonumber(currentValue) <= tonumber(rate) then " |
Member
There was a problem hiding this comment.
The cause should be fixed in releaseAsync() method
Contributor
Author
There was a problem hiding this comment.
Thank you for your answer. I agree that releaseAsync() is the root cause. I'll revert the change and try fixing releaseAsync() instead.
Contributor
Author
There was a problem hiding this comment.
I've updated releaseAsync() and reverted tryAcquireAsync(). The tests pass with these changes. Please review it again when you get a chance.
Revert the change in tryAcquireAsync(). The root cause was that releaseAsync() did not remove the released permits from the zset. As a result, the leftover entries remained in the zset and were reapplied when they expired, causing the available permit count to become inconsistent. releaseAsync() now removes permits from the zset by the released amount, starting with the oldest entries. Signed-off-by: likerhythm <[email protected]>
Signed-off-by: likerhythm <[email protected]>
mrniko
requested changes
Sep 16, 2026
|
|
||
| + "local toRelease = tonumber(ARGV[1]);" | ||
| + "local values = redis.call('zrange', permitsName, 0, -1, 'withscores');" | ||
| + "for i = #values - 1, 1, -2 do " |
Member
There was a problem hiding this comment.
the order is: newest first but it should be oldest first.
Contributor
Author
There was a problem hiding this comment.
I fixed the order and moved the zrem call out of the if-else. Could you take another look? Thanks!
mrniko
reviewed
Sep 16, 2026
| + " local score = values[i + 1];" | ||
| + " local random, permits = struct.unpack('Bc0I', v);" | ||
| + " if permits <= toRelease then " | ||
| + " redis.call('zrem', permitsName, v);" |
Signed-off-by: likerhythm <[email protected]>
Signed-off-by: likerhythm <[email protected]>
Member
|
Thanks for contribution |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #7342
Problem
RedissonRateLimiter.tryAcquireAsync()recalculatescurrentValueasrate - usedwhenevercurrentValue + released > rate. This discards permits that were returned byrelease().Consider a
RedissonRateLimiterwithrate=10andinterval=1000msin the following situation.tryAcquire(1)andrelease(1)nine times at 100ms intervals. Becauserelease()does not touch the sorted set,currentValueis restored to 10 each time and ends atcurrentValue=10, while nine unexpired permit entries accumulate in the sorted set.tryAcquire(1)expires the first permit, soreleased=1. Once that entry is removed, eight entries remain, givingused=8. SincecurrentValue(10) + released(1) > rate(10), the recalculation path runs andcurrentValueis reset torate - used = 2.release(1)are lost, and the available permit count collapses from 10 to 2.Root Cause
A permit returned by
release()is immediately reflected incurrentValue, but not inused. As a result, recalculatingcurrentValueasrate - usedoverwrites and loses the permits that have already been released.Fix
Skip the
rate - usedrecalculation whenused + currentValuealready exceedsrate. That condition indicatescurrentValuehas been inflated byrelease(), so skipping the recalculation preserves the returned permits.Testing
Added a regression test,
testReleasedPermitsPreservedAfterExpiry, that reproduces the case where permits returned byrelease()are lost through therate - usedrecalculation on expiry. The test fails on the current master branch and passes with this change.