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.

Sign up for GitHub

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

Jump to bottom

permitCount paramater values larger than 1 are currently not supported #36

Closed
syntx opened this issue Feb 8, 2023 · 4 comments · Fixed by #159 or #160 · May be fixed by #49
Closed

permitCount paramater values larger than 1 are currently not supported #36

syntx opened this issue Feb 8, 2023 · 4 comments · Fixed by #159 or #160 · May be fixed by #49

Comments

@syntx
Copy link

syntx commented Feb 8, 2023

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:

aspnetcore-redis-rate-limiting/src/RedisRateLimiting/SlidingWindow/RedisSlidingWindowRateLimiter.cs

Lines 51 to 59 in 808c7d8

protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, CancellationToken cancellationToken)
{
if (permitCount > _options.PermitLimit)
{
throw new ArgumentOutOfRangeException(nameof(permitCount), permitCount, string.Format("{0} permit(s) exceeds the permit limit of {1}.", permitCount, _options.PermitLimit));
}
return AcquireAsyncCoreInternal();
}

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

aspnetcore-redis-rate-limiting/src/RedisRateLimiting/FixedWindow/RedisFixedWindowManager.cs

Lines 56 to 67 in bdca17f

var response = (RedisValue[]?)await database.ScriptEvaluateAsync(
_redisScript,
new
{
rate_limit_key = RateLimitKey,
expires_at_key = RateLimitExpireKey,
next_expires_at = now.Add(_options.Window).ToUnixTimeSeconds(),
current_time = nowUnixTimeSeconds,
increment_amount = 1D,
});
var result = new RedisFixedWindowResponse();

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.
@guynoga47
Copy link

An additional missing feature resulting from the same problem is as follows: 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.

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

@cristipufu
Copy link
Owner

@guynoga47 please open a separate issue for your scenario and link the definitions

@manuelspezzani
Copy link

@cristipufu do you plan to support this? Are you willing to accept a pull request?

@cristipufu
Copy link
Owner

@manuelspezzani yes of course

@manuelspezzani manuelspezzani mentioned this issue Apr 7, 2023
Open
This was referenced May 15, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@manuelspezzani @syntx @cristipufu @guynoga47