8000 bug #49070 [RateLimiter] fix incorrect retryAfter of FixedWindow (Rob… · andersmateusz/symfony@b899365 · GitHub
[go: up one dir, main page]
Skip to content

Commit b899365

Browse files
fabpotfabpot
committed
bug symfony#49070 [RateLimiter] fix incorrect retryAfter of FixedWindow (RobertMe)
This PR was merged into the 5.4 branch. Discussion ---------- [RateLimiter] fix incorrect retryAfter of FixedWindow | Q | A | ------------- | --- | Branch? |5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | <!-- Replace this notice by a short README for your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the latest branch. - For new features, provide some code snippets to help understand usage. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> A FixedWindow always ends `intervalInSeconds` after the start time (`timer`). So when calculating the time to consume some tokens the tokens will always be available at `timer + intervalInSeconds`. But currently this is reported incorrectly as `calculateTimeForTokens()` calculates the time based on the desired amount of tokens (and cycles) while it doesn't take into account `maxSize` amount of tokens become available at the windows end. Furthermore calculating the amount of needed cycles is incorrect. This as all tokens become available at once (at the windows end) and you can't consume more tokens than `maxSize` (which is validated at the start of `FixedWindowLimiter::reserve`, in case of `tokens > limit` it throws). **Note:** I don't think that changing the signature of `calculateTimeForTokens` is a deprecation. This as the `Window` class is marked as ``@internal``. So it should only be used by the `RateLimiter` component. Commits ------- 2316932 [RateLimiter] fix incorrect retryAfter of FixedWindow
2 parents 23da9db + 2316932 commit b899365

File tree

3 files changed

+7
-5
lines changed

3 files changed

+7
-5
lines changed

‎src/Symfony/Component/RateLimiter/Policy/FixedWindowLimiter.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public function reserve(int $tokens = 1, float $maxTime = null): Reservation
6868

6969
$reservation = new Reservation($now, new RateLimit($window->getAvailableTokens($now), \DateTimeImmutable::createFromFormat('U', floor($now)), true, $this->limit));
7070
} else {
71-
$waitDuration = $window->calculateTimeForTokens($tokens);
71+
$waitDuration = $window->calculateTimeForTokens($tokens, $now);
7272

7373
if (null !== $maxTime && $waitDuration > $maxTime) {
7474
// process needs to wait longer than set interval

‎src/Symfony/Component/RateLimiter/Policy/Window.php

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,15 +75,13 @@ public function getAvailableTokens(float $now)
7575
return $this->maxSize - $this->hitCount;
7676
}
7777

78-
public function calculateTimeForTokens(int $tokens): int
78+
public function calculateTimeForTokens(int $tokens, float $now): int
7979
{
8080
if (($this->maxSize - $this->hitCount) >= $tokens) {
8181
return 0;
8282
}
8383

84-
$cyclesRequired = ceil($tokens / $this->maxSize);
85-
86-
return $cyclesRequired * $this->intervalInSeconds;
84+
return (int) ceil($this->timer + $this->intervalInSeconds - $now);
8785
}
8886

8987
public function __serialize(): array

‎src/Symfony/Component/RateLimiter/Tests/Policy/FixedWindowLimiterTest.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ protected function setUp(): void
3737

3838
public function testConsume()
3939
{
40+
$now = time();
4041
$limiter = $this->createLimiter();
4142

4243
// fill 9 tokens in 45 seconds
@@ -51,6 +52,9 @@ public function testConsume()
5152
$rateLimit = $limiter->consume();
5253
$this->assertFalse($rateLimit->isAccepted());
5354
$this->assertSame(10, $rateLimit->getLimit());
55+
// Window ends after 1 minute
56+
$retryAfter = \DateTimeImmutable::createFromFormat('U', $now + 60);
57+
$this->assertEquals($retryAfter, $rateLimit->getRetryAfter());
5458
}
5559

5660
/**

0 commit comments

Comments
 (0)
0