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

Add a Redis-backed token bucket RateLimiter implementation #41861

Open
halter73 opened this issue May 26, 2022 · 8 comments
Open

Add a Redis-backed token bucket RateLimiter implementation #41861

halter73 opened this issue May 26, 2022 · 8 comments
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@halter73
Copy link
Member

halter73 commented May 26, 2022

Is your feature request related to a problem? Please describe the problem.

In the runtime repo, we have included a bunch of built-in, in-memory RateLimiter implementations like ConcurrencyLimiter, FixedWindowRateLimiter, SlidingWindowRateLimiter and TokenBucketRateLimiter. It would be nice to add a rate limiter that works across multiple hosts in a distributed environment. In the past, for things like our RedisCache IDistributedCache implementation, we've used StackExchange.Redis to implement a solution in the aspnetcore repo for a runtime abstraction. I think we should do the same things for this.

Describe the solution you'd like

I think we should implement a token bucket RateLimiter based on Redis using StackExchange.Redis that can be used in distributed environments. I imagine the options for this rate limiter will be a combination of what's in TokenBucketRateLimiterOptions and RedisCacheOptions.

We might want to make it easier to reuse existing IConnectionMultiplexer since there will likely be multiple instances by default. Another way to avoid unnecessary Redis connections might be to implement PartitionedRateLimiter<string> instead of RateLimiter.

@BrennanConroy @wtgodbe

Additional context

Customers are asking for this.

@ghost
Copy link

ghost commented May 26, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@adityamandaleeka
Copy link
Member

Worth considering this scenario when working on this too: #41861

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
@Expecho
Copy link

Expecho commented Jan 11, 2024

Is this now moved to .Net 9 or what is the current status?

@amcasey
Copy link
Member

amcasey commented Feb 14, 2024

@mgravell Does this align with the Redis work you're planning?

@ReubenBond
Copy link
Member

BTW, I have proof-of-concept implementations here, which might serve as inspiration (or example of what not to do - either way is fine to me): https://github.com/ReubenBond/DistributedRateLimiting.Redis/tree/main/DistributedRateLimiting.Redis

@mgravell
Copy link
Member

mgravell commented Feb 15, 2024

@amcasey this is certainly a topic I've discussed with Aditya, but this isn't what I'm focusing on right now; I have thoughts on it, in particular around avoiding latency overheads via broadcast, but that also depends on how strict we need to be ("N per T" is very different depending on whether there's an implied "give-or-take" hand-wave in there), but I haven't actively spiked anything. I'll take a look at Reuben's braindump.

(after eyeball) @ReubenBond the approach itself looks fine (I think it still pays latency for the core acquire, but maybe that's OK in most scenarios), but purely meant constructively: there are a few things in that impl that can be improved, in particular around redis compatibility re cluster routing and side-effect vs verbatim script replication; I suspect you tested on a standalone (non-cluster) newish server, which would have hidden some landmines from you - but: they exist. Happy to take a moment to discuss these at your convenience, if you want to learn a little more redis nuance.

@md-redwan-hossain
Copy link

Is there any progress of this feature?

@adityamandaleeka
Copy link
Member

Hey @md-redwan-hossain, we haven't worked more on this since the last update. We're almost certainly not going to have time for this in the .NET 9 timeframe.

We're definitely aware of the need for a better distributed story for rate limiting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

No branches or pull requests

9 participants