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
Share limits between several routes #8
Conversation
Did not intend to share that second pull request under the same issue #. |
@g-p-g : thanks for this. I like this idea of making a limit optionally named and shared. I'd like to propose an alternate implementation which could be used as follows:
I'm also toying with the idea of allowing the scope parameter to be either a string or a callable (that returns a scope string) which would allow the scope to be configurable at runtime. (this part could be implemented on top of either implementation). What do you think? |
I like the idea of creating shared limits to avoid using wrong scopes, but then I would favour something like The scope being able to be a function is something potentially interesting, too. While I don't have a use case for that at this moment, I can imagine some people would like to have fun deciding which scope to apply on the fly. |
@g-p-g: I'm ok with the using an explicit decorator for shared limits. re: what happens when the scope isn't explicitly specified by the user, I'm using a unique id. Please take a look at 353ab2e for my take on this, and if it works for you please rebase your master with that branch (alisaifee/shared_limits) so the commits still stay part of this PR and ill merge 'em in. |
@alisaifee I'm happy with the solution, I like that you update the docs too :) Regarding the updated doc, I see some examples are using limit.limit while others are using limit.shared_limit. Should all these new ones use shared_limit instead? |
good catch, thank you. fixed at 0065f52 |
@alisaifee thanks. It looks like there's some issue with the unique id idea, as I can go over the limit if I don't explicitly specify a scope. |
@alisaifee I didn't look much into the code, but here's what I think will cause trouble when not specifying the scope: what happens if I start multiple instances of a given server under a single redis instance? I believe it's going to generate the "unique" id several times, since in fact there are multiple instances for the limiter. |
you're absolutely right - i'd completely ignored the case of multiple servers running the same instance. I guess having a scope name as a requirement is the only way to go, yeah? |
I would think so, at least that's an obvious way to do it. Maybe some documentation to describe way that's so could help others in understanding the issue. |
This doesn't work for multiple app instances running since each instance will generate a new UUID for the same shared_limit.
rebased, thanks for supporting the idea. |
@g-p-g: you're most welcome. I'll probably release this in a couple of hours - if its ok with you could you provide me with your name, so I can add you to the contributors list? |
Share limits between several routes
Guilherme Polo is the name I usually use. |
great, thanks! |
It looks like the current implementation does not allow sharing a single custom rate limit among several routes (global limits excluded).
For instance, take the example where you have a /slow route limited to 1 request per day. What if I have a second route which I would like to share the limit of 1 request per day along the other /slow route? Right now the code will limit each route to 1 req per day, while it would be useful to allow 1 req per day for all the slow routes combined.