Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit b636e66

Browse filesBrowse files
committed
bug #40141 [RateLimiter] Fix sliding_window misbehaving with stale records (xesxen)
This PR was squashed before being merged into the 5.2 branch. Discussion ---------- [RateLimiter] Fix sliding_window misbehaving with stale records | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Currently the SlidingWindow RateLimiter returns a negative value for getHitCount if the previous SlidingWindow was too long ago. This results in a really high value from `SlidingWindowLimiter::getAvailableTokens()` which is higher than the configured limit. This limits the value of percentOfCurrentTimeframe in `SlidingWindow::getHitCount()` to 1 so it can't result in a negative hitcount. The 2nd fix fixes the SlidingWindow instance (essentially) not storing hits if the previous instance is way in the past, as the next instance will still be "in the past". This causes RateLimit to behave as if it were disabled until it has caught up again, which could take a long time when it is configured with a small window size. Commits ------- 5703316 [RateLimiter] Fix sliding_window misbehaving with stale records
2 parents 9fa7dbd + 5703316 commit b636e66
Copy full SHA for b636e66

File tree

2 files changed

+43
-3
lines changed
Filter options

2 files changed

+43
-3
lines changed

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Policy/SlidingWindow.php
+7-3Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,12 @@ public function __construct(string $id, int $intervalInSeconds)
6363
public static function createFromPreviousWindow(self $window, int $intervalInSeconds): self
6464
{
6565
$new = new self($window->id, $intervalInSeconds);
66-
$new->hitCountForLastWindow = $window->hitCount;
67-
$new->windowEndAt = $window->windowEndAt + $intervalInSeconds;
66+
$windowEndAt = $window->windowEndAt + $intervalInSeconds;
67+
68+
if (time() < $windowEndAt) {
69+
$new->hitCountForLastWindow = $window->hitCount;
70+
$new->windowEndAt = $windowEndAt;
71+
}
6872

6973
return $new;
7074
}
@@ -112,7 +116,7 @@ public function add(int $hits = 1)
112116
public function getHitCount(): int
113117
{
114118
$startOfWindow = $this->windowEndAt - $this->intervalInSeconds;
115-
$percentOfCurrentTimeFrame = (time() - $startOfWindow) / $this->intervalInSeconds;
119+
$percentOfCurrentTimeFrame = min((time() - $startOfWindow) / $this->intervalInSeconds, 1);
116120

117121
return (int) floor($this->hitCountForLastWindow * (1 - $percentOfCurrentTimeFrame) + $this->hitCount);
118122
}

‎src/Symfony/Component/RateLimiter/Tests/Strategy/SlidingWindowTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Tests/Strategy/SlidingWindowTest.php
+36Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,13 @@
1212
namespace Symfony\Component\RateLimiter\Tests\Policy;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Symfony\Bridge\PhpUnit\ClockMock;
1516
use Symfony\Component\RateLimiter\Exception\InvalidIntervalException;
1617
use Symfony\Component\RateLimiter\Policy\SlidingWindow;
1718

19+
/**
20+
* @group time-sensitive
21+
*/
1822
class SlidingWindowTest extends TestCase
1923
{
2024
public function testGetExpirationTime()
@@ -36,4 +40,36 @@ public function testInvalidInterval()
3640
$this->expectException(InvalidIntervalException::class);
3741
new SlidingWindow('foo', 0);
3842
}
43+
44+
public function testLongInterval()
45+
{
46+
ClockMock::register(SlidingWindow::class);
47+
$window = new SlidingWindow('foo', 60);
48+
$this->assertSame(0, $window->getHitCount());
49+
$window->add(20);
50+
$this->assertSame(20, $window->getHitCount());
51+
52+
sleep(60);
53+
$new = SlidingWindow::createFromPreviousWindow($window, 60);
54+
$this->assertSame(20, $new->getHitCount());
55+
56+
sleep(30);
57+
$this->assertSame(10, $new->getHitCount());
58+
59+
sleep(30);
60+
$this->assertSame(0, $new->getHitCount());
61+
62+
sleep(30);
63+
$this->assertSame(0, $new->getHitCount());
64+
}
65+
66+
public function testLongIntervalCreate()
67+
{
68+
ClockMock::register(SlidingWindow::class);
69+
$window = new SlidingWindow('foo', 60);
70+
71+
sleep(300);
72+
$new = SlidingWindow::createFromPreviousWindow($window, 60);
73+
$this->assertFalse($new->isExpired());
74+
}
3975
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.