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

The RedisTokenBucketRateLimiter refills the bucket immediately after some timeout #46

Closed
Namoshek opened this issue Mar 21, 2023 · 2 comments · Fixed by #47
Closed

The RedisTokenBucketRateLimiter refills the bucket immediately after some timeout #46

Namoshek opened this issue Mar 21, 2023 · 2 comments · Fixed by #47

Comments

@Namoshek
Copy link
Contributor

Namoshek commented Mar 21, 2023

According to the documentation, the RedisTokenBucketRateLimiter is supposed to refill the bucket by TokensPerPeriod every ReplenishmentPeriod until TokenLimit is reached. Because refilling the bucket is no constant process, this is done whenever the rate limit is accessed through the Lua script.

To prevent stale rate limit entries in Redis, the Lua script calculates and sets a TTL for the rate limit keys:

local fill_time = limit / rate
local ttl = math.floor(fill_time * 2)

The calculated TTL, however, does not seem to be correct. fill_time is the number of ReplenishmentPeriods it takes, until the bucket is full again. This leads to a valid TTL if ReplenishmentPeriod is less than 2 seconds. But in case the ReplenishmentPeriod is higher, like 15 seconds, the TTL is wrong. Which, in case no requests occur for the duration of the TTL, leads to a bucket which is filled early.

Example of a problematic rate limiter:

builder.Services.AddRateLimiter(options =>
{
    options.AddRedisTokenBucketLimiter("MyLimit", (opt) =>
    {
        opt.ConnectionMultiplexerFactory = () => connectionMultiplexer;
        opt.TokenLimit = 3;
        opt.TokensPerPeriod = 1;
        opt.ReplenishmentPeriod = TimeSpan.FromSeconds(15);
    });
});

The fix for this should be simple, but I'll have to look into it.

@Namoshek
Copy link
Contributor Author

While looking into this issue a bit more, I also discovered another issue:
When the ReplenishmentPeriod is higher than the interval in which clients access the API, each request will update the @timestamp_key in the database even though @rate_limit_key remains the same. The latter is because current_tokens is always an integer and math.floor() is used to avoid granting too many tokens. The result, however, is that a client which exceeded his quota and keeps making requests is locking himself out infinitely.

I'm actually surprised that no one else noticed so far...

@alex-zissis
Copy link

Thanks! I noticed this too and couldn't work it out. I pulled down your fork and it works as expected.
Would love if @cristipufu could have a look at it.

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 a pull request may close this issue.

2 participants