-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
[5.5] Add Redis limiters #20597
[5.5] Add Redis limiters #20597
Conversation
This looks like it would only work with redis. Is there any hope of implementing it for any other cache technology supported by Laravel? |
Redis guarantees atomicity, if you have ideas for support for other drivers you can always PR that. |
I'm worried that we're moving further and further to redis-only functionality. I would argue that if this should be in the framework, it should be supported by more underlying systems. As an example, the existing RateLimiter implementation supports all cache implementations. Also, couldn't this just be provided as a separate package? |
@sisve you can always PR support for another driver :) |
I have no idea how to respond without sounding like the usual old grumpy hostile me. In short; it's not up to me to provide a complete implementation of a PR you've written. It's either 1) up to someone higher up (read: Taylor) to decide if a redis-only functionality is something that should be in the framework, or 2) up to you to provide a complete implementation of the feature. There are several places you can ask for help or find more contributors to write your code, but I do not believe that it is, as-is, ready to be merged into the framework. |
I had been thinking about how to implement something like this the past couple days so I'm really happy to see this code but I see sisve's point here. One of the best parts of Laravel is the ability to swap out drivers easily and (mostly) have it work out alright. I think Horizon being redis specific is fine because it's a separate package with it's own dependencies. Again, I really like the idea of this but is it possible that it's better off in a package? |
I'm fine with having it in a separate non-laravel-maintained package on my own, but I don't really find it a problem to have these two Redis features under the |
I agree @themsaid -- It's solely in the Redis namespace and isn't being advertised as something generic. If someone wants to take this and abstract it out to work with other backends so be it. Better to have this sooner than wait for a "perfect" solution that may never come to fruition. |
Very well explained. Thanks! |
Except that it is advertised as the universal rate limit solution. See laravel/ideas#242 as a reference.
|
And thats still true. It never said it allowed you to store those rate limits in the object store of your choice... |
IMO all our cache drivers should adhere to the same contract, and provide the same functionality. A framework should not be opinionated about which cache I choose to use, but this PR makes it so that you need to use a certain cache driver to use certain features. I, like @sisve, would prefer this in a separate package. I think it makes a lot of sense as a separate package, if desired it could even be placed under the Laravel namespace and linked to in the docs, but I think it's wrong to provide more functionality for a given engine, just because you're not sure how to implement it for other drivers. If there are actual constraints that can be provided, which prevent this from being in anything but Redis, then sure, I guess it's sort of ok. However, I do believe it's entirely possible to implement for e.g Memcached using @tomschlick it's not a universal rate limit solution when you apply constraints. Universal would imply anyone can use it, which just isn't true. I'm saying all this because I don't want the framework to move in a direction where we start dropping support for multiple drivers, just to be able to use one cool feature; that's what packages are for. |
Well Laravel already has some functionalities that only work for certain drivers, such as cache tagging. I don't see this as a bad thing ... if we keep doing this we are only able to use the intersection of all drivers functionalities... |
@phroggyy this has nothing to do with cache. It's not even in the Cache namespace. This is a redis specific feature. If we want to create a driver based |
@themsaid Just want to mention with Redis, it is possible to not put sleep() into the code and make the wait algorithm faster. I have a code for mutual exclusive lock. Please take a look: |
@themsaid where is the right place in my application to setup up the limiter? I have three different jobs which do the same external api call which is limited to one request per seconds. How can I manage throttling via redis queue? Thanks in advance! |
@florianflock I had the same question. I have an "email" queue that I would like to push jobs onto and have the entire queue rate limited with one command. Unfortunately I haven't found a way to do it. I accomplished the rate limiting by adding the Redis::throttle function to the handle of every job I have that sends email like so:
In this case, |
Thanks @stevencook, may I try to build a service class which does that throttling. Something like that |
@stevencook I think release accepts a $delay parameter in seconds at /vendor/laravel/framework/src/Illuminate/Queue/InteractsWithQueue.php::release which in turn calls /vendor/laravel/framework/src/Illuminate/Queue/Jobs/RedisJob.php::release Eventually gets to this point
I'm not sure if that helps at all but just testing throttling right now |
This PR adds two types of limiters:
These limiters can be used in a middleware to throttle access to certain routes, or in a queued job to limit hitting a 3rd party service or even prevent a race condition of two processes trying to update a single resource and would cause a race condition.
The code above will ensure only a single task is touching the nginx file, it can be added in multiple places in your code and it'll protect your file from race conditions.
In case no available slots, the limiter will try again for 5 seconds before an exception is thrown.
If a slot was reserved for more than 10 seconds it'll be automatically freed so it can be used by another task.