cristipufu / aspnetcore-redis-rate-limiting Goto Github PK

Hello,

That is a great package. I have tested with multiple instances of same api behind a loadbalancer. It seems to be working well. But have you tried it with high volume of concurent/paralel requests. Is the counter atomic? And also when you hit the limit, it increments the counter value by number of instances.
Also , its response time is above 2000ms when using client based limiter with multiple instances of app.
Thanks :)

I wanted to use this library and did some testing. This was my code:

`services.AddRateLimiter(options =>
{
options.OnRejected = (context, _) =>
{
if (context.Lease.TryGetMetadata(MetadataName.RetryAfter, out var retryAfter))
{
context.HttpContext.Response.Headers.RetryAfter =
((int)retryAfter.TotalSeconds).ToString(NumberFormatInfo.InvariantInfo);
}

    context.HttpContext.Response.StatusCode = StatusCodes.Status429TooManyRequests;
    context.HttpContext.Response.WriteAsync("Too many requests. Please try again later.");

    return new ValueTask();
};
options.GlobalLimiter = PartitionedRateLimiter.CreateChained(
        PartitionedRateLimiter.Create<HttpContext, string>(httpContext =>
        {
            string clientId = GetClientFromContext(httpContext);
            return RedisRateLimitPartition.GetFixedWindowRateLimiter(clientId, _ =>
               new RedisFixedWindowRateLimiterOptions
               {
                   ConnectionMultiplexerFactory = () => connectionMultiplexer,
                   PermitLimit = 2,
                   Window = TimeSpan.FromSeconds(1)                                   
               });
        }),
        PartitionedRateLimiter.Create<HttpContext, string>(httpContext =>
        {
            string clientId = GetClientFromContext(httpContext);
            return RateLimitPartition.GetNoLimiter(clientId);
        }),
        PartitionedRateLimiter.Create<HttpContext, string>(httpContext =>
        {
            string clientId = GetClientFromContext(httpContext);
            return RedisRateLimitPartition.GetConcurrencyRateLimiter(clientId, _ =>
               new RedisConcurrencyRateLimiterOptions
               {
                   ConnectionMultiplexerFactory = () => connectionMultiplexer,
                   PermitLimit = 5,
                   QueueLimit = 10
               });
        })
        );

});`

It worked as expected for a while, and at some point I started getting this error from Redis:

"ERR Error running script (call to f_a08ae7b80fbefc1d082f3c02f112bb4f38a59fa7): @user_script:8: WRONGTYPE Operation against a key holding the wrong kind of value"

I looked in Redis monitor and saw the error as a result of this command:
"evalsha" "a08ae7b80fbefc1d082f3c02f112bb4f38a59fa7" "3" "rl:{tabdevweb.ini}" "rl:{tabdevweb.ini}:q" "rl:{tabdevweb.ini}:stats" "2" "1000" "0" "1693226580" "rl:{tabdevweb.ini}" "rl:{tabdevweb.ini}:q" "b2ec810f-80ac-4c50-b2b9-e46aad0de33c" "rl:{tabdevweb.ini}:stats"

After a while (next day) the error disappeared and now its working fine again.

Thanks

RedisSlidingWindowManager uses the EXPIREAT redis call to update the expiry every time it attempts to acquire a lease. This function only accepts a whole number of seconds as its argument. However the expression (RedisValue)_options.Window.TotalSeconds, is used when passing the window size into the lua script where TotalSeconds is a double.

This usually works because RedisValue turns double into an integer if it can be exactly represented. But if a window size with sub-second precision is specified, the value is passed as a double and an error occurs.

The obvious fix is to just cast total seconds to long to truncate it before conversion to a RedisValue. Alternatively if sub-second precision is actually wanted Redis also supports millisecond precision for expiry through PEXPIREAT since version 2.6.0.

If I know which option is preferred I can create a PR.

Using AWS ElasticCache:

StackExchange.Redis.RedisServerException: ERR Error running script (call to f_d3a858ae047422548b35753d09e9d2fe57ec91c0): @user_script:2: @user_script: 2: Lua script attempted to access a non local key in a cluster node
   at StackExchange.Redis.ConnectionMultiplexer.ExecuteSyncImpl[T](Message message, ResultProcessor`1 processor, ServerEndPoint server, T defaultValue) in /_/src/StackExchange.Redis/ConnectionMultiplexer.cs:line 1909
   at StackExchange.Redis.RedisDatabase.ScriptEvaluate(String script, RedisKey[] keys, RedisValue[] values, CommandFlags flags) in /_/src/StackExchange.Redis/RedisDatabase.cs:line 1501
   at StackExchange.Redis.RedisDatabase.ScriptEvaluate(LuaScript script, Object parameters, CommandFlags flags) in /_/src/StackExchange.Redis/RedisDatabase.cs:line 1536
   at RedisRateLimiting.Concurrency.RedisFixedWindowManager.TryAcquireLease()
   at RedisRateLimiting.RedisFixedWindowRateLimiter`1.AttemptAcquireCore(Int32 permitCount)

When trying to load test the sliding window limiter I noticed that it quickly lead to redis timeouts especially in environments with limited cpu count and/or and a bit of latency to redis. This seems to be caused by exhausting the thread pool. Looking into the issue I found the following: The TryAcquireAsync function in RateLimitingMiddleware.cs first does a synchronous AttemptAcquire call before falling back to before falling back to AcquireAsync on the RateLimiter. This means unless something goes wrong, only synchronous StackExchange Redis calls are performed by this package.

Looking at the documentation on the RateLimiter class it says:

AcquireAsync(Int32, CancellationToken) Wait until the requested permits are available or permits can no longer be acquired.
AttemptAcquire(Int32) Fast synchronous attempt to acquire permits.

So this isn't just the usual case of having an async and a blocking implementation but meant to be distinct functions that can both be used.

I have to admit that I am not sure I fully get the intent of the interface with regards to waiting until a permit is available. But I think the way to get proper scalability would probably be to never return a lease from the synchronous call and only reach out to redis in the asynchronous one.

Would this be an appropriate change? Bit surprised I am the first one to stumble over this so maybe I am on the wrong track completely.

Hi @cristipufu,

My team is preparing to upgrade from .Net6 to .Net8 (due for GA in November) and I noticed this library is .Net7 only. Do you have any plans to add .Net8 support soon?

If not, is there anything holding you back where perhaps we could be of help?

Cheers, Jeroen

RedisTokenBucketRateLimiter throws an error on Amazon ElastiCache Serverless for Redis. Every other limiter type works just fine.

StackExchange.Redis.RedisServerException: ERR This Redis command is not allowed from script script: 53aa7d296ba9ded783301cc275161b6e344ad383, on @user_script:13.
      at StackExchange.Redis.RedisDatabase.ScriptEvaluateAsync(String script, RedisKey[] keys, RedisValue[] values, CommandFlags flags) in /_/src/StackExchange.Redis/RedisDatabase.cs:line 1551
      at RedisRateLimiting.Concurrency.RedisTokenBucketManager.TryAcquireLeaseAsync()
      at RedisRateLimiting.RedisTokenBucketRateLimiter`1.AcquireAsyncCoreInternal()

What happens if we can't connect to Redis?
Should we fail the request?
Should we swallow the exception?
Should we configure the behavior?

dotnet/aspnetcore#44907

[EnableRateLimiting("policy_fixed_window", "policy_concurrency")]
public class DemoController : ControllerBase

Use RateLimiter.GetStatistics() + OpenTelemetry.Metrics

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:

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.

The current implementation always returns TimeSpan.Zero for RateLimiter.IdleDuration. When using a partitioned rate limiter this means the rate limiters created for the partitions never get cleaned up and will accumulate.

I think this is true for all rate limiters in this repo however as it is the only one I am using right now, I only looked at the sliding window rate limiter in detail so far. For that the most reasonable approach to me seems to be to accept some imprecision and just track the last expireat value set in the RedisSlidingWindowManager and provide an estimated idle duration for that. This is under the assumption that if a manager ever deletes a partition that was not actually fully idle, it is really no problem to have it just be re-created by the factory. The actual limit is still in redis after all.

I could create a pull request for the redis sliding rate limiter if this approach is acceptable.

This would not fully match what the interface wants though. The documentation says:

Specifies how long the RateLimiter has had all permits available.

A better estimate or precise answer could of course be given by reaching out to redis, however to perform potentially blocking operations in there seems problematic to me.

The parameter permitCount is passed to both AttemptAcquire and AcquireAsync in the RateLimiter abstract base class that all rate limiters in this library are are implementing.

The definition for this paramater is as follows:
<param name="permitCount">Number of permits to try and acquire.</param>
(See for instance here: https://github.com/dotnet/runtime/blob/43a60c8ed073a4c6134facadd01c9c1c2643e41a/src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/RateLimiter.cs#L60)

Yet, all the provided classes disregard this parameter value, as in here:

In some cases, a hard coded value of 1D is then passed on instead of the parameter, as in here:

Are there plans to solve this?
Also, If this is currently a known limitation of this library (fair), please provide a warning in the documentation.

Thanks.

I'm using a Token Bucket Rate Limiting encountered an Error ERR Error running script (call to f_53aa7d296ba9ded783301cc275161b6e344ad383) : @user_script:10: user_script:10: attempt to call field 'replicate_commands' (a nil value)

Hi,

Will it be possible to manage chained limiters in future releases?

Thanks in advance.

According to the definitions of AttemptAcquire and AcquireAsync, when given permitCount = 0, it is possible to check if the permits are exhausted or wait until the permits are replenished, respectively.

Here are the links to the definitions:

https://learn.microsoft.com/en-us/dotnet/api/system.threading.ratelimiting.ratelimiter.attemptacquire?view=aspnetcore-7.0

https://learn.microsoft.com/en-us/dotnet/api/system.threading.ratelimiting.ratelimiter.acquireasync?view=aspnetcore-7.0

I would be happy to know if there is any workaround to check the actual state of the permits without acquiring a lease.

Being able to apply Rules to a controller's action by just defining endpoint and limit settings in appsettings.json similar to how it is done in AspNetCoreRateLimit.

Want to be able to dynamically assign rules to each action instead of having to manually doing it (as some projects are too big to do it manually)

dotnet/runtime#77669

private void Release(ConcurencyLeaseContext leaseContext)
{
    var database = _connectionMultiplexer.GetDatabase();
    // how to use async? if only RateLimitLease would implement IAsyncDisposable
    database.SortedSetRemove($"rl:{_partitionKey}", leaseContext.RequestId);
}

Hey,

thanks for providing this useful package. I was wondering if there is a way to configure a prefix for the redis key? Currently, it could lead to problems if several applications share the same redis database.

Thanks in advance.

Custom middleware

public interface IRateLimiterPolicy<TPartitionKey>
{
    Func<OnRejectedContext, CancellationToken, ValueTask>? OnRejected { get; }
+   Func<OnAcquiredContext, CancellationToken, ValueTask>? OnAcquired { get; }
}

public sealed class RateLimiterOptions
{
    public Func<OnRejectedContext, CancellationToken, ValueTask>? OnRejected { get; set; }
+    public Func<OnAcquiredContext, CancellationToken, ValueTask>? OnAcquired { get; set; }
}

dotnet/aspnetcore#44140

A header "X-Rate-Limit-Reset" containing for example "2023-04-21T11:21:43.6820378Z" would be a nice addition.

Since the data is available on the RedisFixedWindowResponse class, it should be possible to add it.

See pull request: #55