Skip to content
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

Fix: RedisTokenBucketRateLimiter does not refill and block randomly anymore #47

Merged
merged 4 commits into from
May 28, 2023
Merged

Fix: RedisTokenBucketRateLimiter does not refill and block randomly anymore #47

merged 4 commits into from
May 28, 2023

Conversation

Namoshek
Copy link
Contributor

Fixes #46

This PR reworks the Lua script used by the RedisTokenBucketManager in a few ways. The primary goal is to fix the issues mentioned in #46. Summary of the changes:

  • The current time now is generated by Redis now, which is safe due to the use of redis.replicate_commands(). By letting Redis take the time itself, all requests are clocked by the same system, guaranteeing best accuracy.
  • All timestamps are in milliseconds now, which is more accurate and allows working with sub-second TimeSpans.
  • redis.call('GET', ...) calls have been replaced with redis.call('MGET', ...) to safe a round-trip to the database.
  • Some variables were renamed in order to give them a clearer name.
  • The new current_tokens together with the timestamp are only persisted, if the request is allowed.
  • The TTL for the persisted data is now calculated from the periods_until_full multiplied by the period (duration). This fixes the first issue described in The RedisTokenBucketRateLimiter refills the bucket immediately after some timeout #46.
  • The persisted timestamp is not now anymore but the time_of_last_replenishment. This fixes the second issue described in The RedisTokenBucketRateLimiter refills the bucket immediately after some timeout #46.

Besides the changes to the Lua script, the PR also includes the following other changes:

  • The parameters passed to Redis in IRedisDatabase.ScriptEvaluate() are now explicitly flagged as (RedisKey) or (RedisValue) respectively. This ensures that the parameters are mapped to the KEYS[] and ARGS[] arrays correctly, which is important for correct script execution (especially in clusters).
  • The Lua script does now calculate retry_after (in milliseconds), which is 0 if the request was allowed or the milliseconds until the next token is generated if not allowed. This value is returned by the rate limiter in rounded up seconds.

Namoshek and others added 4 commits March 21, 2023 22:27
The worst case is something like (limit: 5, rate: 2) where it takes 3 periods to fill the bucket from 0 to 5. As we do not perform partial replenisments, `math.ceil()` is needed to get the correct number of replenisments needed.
@Namoshek
Copy link
Contributor Author

The failed build is most likely due to the workflow not having access to the Redis connection string secret. The reason for this is explained in this GitHub blog post, where they describe that pull requests could potentially get access to secrets when using the pull_request workflow trigger type. If the pull_request_target workflow trigger type is used instead, the build runs against the build definition of the pull request target branch and secrets are made available automatically.

@cristipufu cristipufu merged commit f0b6c1c into cristipufu:master May 28, 2023
@cristipufu
Copy link
Owner

@Namoshek thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The RedisTokenBucketRateLimiter refills the bucket immediately after some timeout
2 participants