-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[RateLimiter] SlidingWindow to use microtime() instead of time() #42194
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
Comments
@wouterj Is there some specific reason we are using seconds? |
I don't personally see a use-case of limiting that requires microsecond precision. Are there such use-cases? |
Not so much for limiting a user's login attempts but for API requests. I think it make sense. Sure, API authors can have the attitude of "just wait a bit longer", but it seams non-optimal for a library to use seconds. Changing to microseconds would also line up with messenger and their delay/retry. So Im on the fence, but Im leaning towards microseconds because it gives more freedom to the super cool "edge case users" with millions of requests per minute. |
Those users should not at all rely on a rate limiter build in php and requiring a full Symfony kernel boot 😅 (as also lined out in the docs, such tooling should be done as close to the edge as possible). In any case, I guess adding it doesn't hurt much and if @jlekowski wants to put some time into making it like that, I guess there is nothing left to really |
@jlekowski please ping me in the pr |
* 5.4: (46 commits) move username/password fix to non-deprecated Connection class cs fix [VarDumper] Fix dumping twig templates found in exceptions do not replace definition arguments that have not been configured fix Console tests on Windows [Validator] Add translations for CIDR constraint [Dotenv] Fix testLoadEnv() to start from a fresh context [Console] Add completion to server:dump command bug #42194 [RateLimiter] fix: sliding window policy to use microtime [Validator] Update validators.sr_Cyrl.xlf [Validator] Update validators.sr_Latn.xlf Add suggestions for the option 'format' of lints commands: twig, yaml and xliff [VarDumper] Add support for Fiber uzb translation Update validators.uz.xlf Fix logging of impersonator introduced in 5.3 [Console] Add show proxified command class in completion debug skip command completion tests with older Symfony Console versions [Uid] Allow use autocompletion [Console] Add completion to messenger:setup-transports command ...
…se microtime - fix test (jlekowski) This PR was merged into the 5.4 branch. Discussion ---------- [RateLimiter] bug #42194 fix: sliding window policy to use microtime - fix test | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #42194 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> As pointed out in the comment #43677 (comment), `RateLimitTest::testWaitUsesMicrotime()` fails intermittently. I have looked at all `PHPUnit` actions since the test was introduced and interestingly, the fails only occurred during `7.2` test, and the time difference was always ~1.3s. Commits ------- e616963 bug #42194 [RateLimiter] fix: sliding window policy to use microtime - fix test
Description
RateLimiter
uses usually full seconds:time()
,sleep()
,\DateTimeImmutable::createFromFormat('U', [time])
.Sometimes however, it goes with microseconds:
microtime(true)
,usleep()
and could use them even more widely:
\DateTimeImmutable::createFromFormat('U.u', [microtime])
,return (float) $interval->format('%f') / 1e6 // microseconds...
,public function getExpirationTime(): ?float;
.Using
float
gives an option to set interval in milliseconds:and also to shorten wait time (e.g. 1.8s instead of 2.0s).
Example
SlidingWindow
usestime()
which means that after running->wait()
it is still considered as not expired.Summary
Before I go with a PR I would like to know if this change/improvement sounds like something you might merge in (i.e. using integers for time is not set in stone). It works for me locally.
The text was updated successfully, but these errors were encountered: