-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix limit middleware skip options #1568
Conversation
https://github.com/nfriedly/express-rate-limit#skipsuccessfulrequests you can have a look how other languages do it, this is only a process problem, not a programming language problem. e.g. in express you should find a solution |
I checked the codes. They decreased the hit counter with a callback at the end of response ( Also in this express library, they put some info about possible problem of this implementation. https://github.com/nfriedly/express-rate-limit#skipsuccessfulrequests
I will think about optimal solution. But I can't promise anything. |
I merged the first iteration |
When will this be released in a new version? |
In the next days. |
Can u guys pls release this a little sooner? this completely breaks the limiter, it simply returns the status code but still executes everything, but we still really need the skip features so rollback is not an option for us. |
I will release the fix right away, just have to wait a short time for the checks |
Please provide enough information so that others can review your pull request:
Related: #1567
Explain the details for making this change. What existing problem does the pull request solve?
This solution
1- doesn't forward the request before the limit reached calculations.
2- doesn't returns 429 on successful requests and doesn't overwrite the actual statuscode.
But there is a problem that I can't solve. I need some help.
When remaining number is 1 and next request should be skipped, this code will return 429 because it can't see the actual status code before checking limit reached.