10000 [RateLimiter] SlidingWindow to use microtime() instead of time() · Issue #42194 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Closed
jlekowski opened this issue Jul 19, 2021 · 6 comments · Fixed by #43677 or #43688
Closed

[RateLimiter] SlidingWindow to use microtime() instead of time() #42194

jlekowski opened this issue Jul 19, 2021 · 6 comments · Fixed by #43677 or #43688
Labels
Feature RateLimiter

Comments

@jlekowski
Copy link
Contributor

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:

var_dump(\DateInterval::createFromDateString('200ms')->format('%f'));
// string(6) "200000"

and also to shorten wait time (e.g. 1.8s instead of 2.0s).

Example
SlidingWindow uses time() which means that after running ->wait() it is still considered as not expired.

<?php

use Symfony\Component\RateLimiter\Policy\SlidingWindow;

require_once './vendor/autoload.php';

$window = new SlidingWindow('some_id', 2);
sleep(2); // mimic $limit->wait();
var_dump($window->isExpired()); // false - same second

$window = new class ('some_id', 2) {
    public function __construct(string $id, int $intervalInSeconds)
    {
        $this->windowEndAt = microtime(true) + $intervalInSeconds;
    }

    public function isExpired(): bool
    {
        return microtime(true) > $this->windowEndAt;
    }
};
sleep(2); // mimic $limit->wait();
var_dump($window->isExpired()); // true

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.
@xabbuh xabbuh added the Feature label Jul 20, 2021
@jlekowski jlekowski mentioned this issue Aug 2, 2021
Merged
@Nyholm
Copy link
Member
Nyholm commented Oct 19, 2021

@wouterj Is there some specific reason we are using seconds?
What about using microseconds everywhere in version 6.0? (ie still using integers)

@wouterj
Copy link
Member
wouterj commented Oct 19, 2021

I don't personally see a use-case of limiting that requires microsecond precision. Are there such use-cases?

@Nyholm
Copy link
Member
Nyholm commented Oct 19, 2021

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.

@jlekowski
Copy link
Contributor Author

@wouterj, @Nyholm, thanks a mill for your time and having a look at this. I will prepare a PR with the changes.

@wouterj
Copy link
Member
wouterj commented Oct 19, 2021

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
"reject" a feature like this.

@Nyholm
Copy link
Member
Nyholm commented Oct 19, 2021

@jlekowski please ping me in the pr

@jlekowski jlekowski mentioned this issue Oct 23, 2021
Merged
@fabpot fabpot closed this as completed in 85bf403 Oct 24, 2021
@jlekowski jlekowski mentioned this issue Oct 25, 2021
Merged
nicolas-grekas added a commit that referenced this issue Oct 25, 2021
@nicolas-grekas
Merge branch '5.4' into 6.0
e97a6f7 
* 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
  ...
chalasr added a commit that referenced this issue Oct 26, 2021
@chalasr
minor #43688 [RateLimiter] bug #42194 fix: sliding window policy to u…
1c79d80 
…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
This was referenced Nov 5, 2021
Merged
Merged
@MatTheCat MatTheCat mentioned this issue Jan 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature RateLimiter
Projects
None yet
Milestone
No milestone
5 participants
@wouterj @jlekowski @Nyholm @xabbuh @carsonbot
0