Skip to content

Navigation Menu

Sign in

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 64f6ac2

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
bug #53259 [RateLimit] Test and fix peeking behavior on rate limit policies (wouterj)
This PR was merged into the 6.3 branch. Discussion ---------- [RateLimit] Test and fix peeking behavior on rate limit policies | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Ref #52835 | License | MIT Although heavily discouraged for user-land code, we've implemented peeking behavior for rate limiting in 6.2 with #46110. However, I found that our rate limit policies show very inconsistent behavior on this. As we didn't have great test covering peeking return values, we broke BC with #51676 in 6.4. I propose this PR to verify the behavior of the policies and also make it inconsistent. I target 6.3 because we rely on this in the login throttling since 6.2 and this shows buggy error messages ("try again in 0 minute") when not using the default policy for login throttling. > [!NOTE] > When merging this PR, there will be heavy merge conflicts in the SlidingWindowLimiter. You can ignore the changes in this PR for this policy in 6.4. I'll rebase and update #52835 to fix the sliding window limiter in 6.4+ Commits ------- e4a8c33 [RateLimit] Test and fix peeking behavior on rate limit policies
2 parents 0fae922 + e4a8c33 commit 64f6ac2
Copy full SHA for 64f6ac2

File tree

6 files changed

+66
-8
lines changed
Filter options

6 files changed

+66
-8
lines changed

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Policy/FixedWindowLimiter.php
+5-2Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,15 @@ public function reserve(int $tokens = 1, float $maxTime = null): Reservation
5959
$now = microtime(true);
6060
$availableTokens = $window->getAvailableTokens($now);
6161

62-
if ($availableTokens >= max(1, $tokens)) {
62+
if (0 === $tokens) {
63+
$waitDuration = $window->calculateTimeForTokens(1, $now);
64+
$reservation = new Reservation($now + $waitDuration, new RateLimit($window->getAvailableTokens($now), \DateTimeImmutable::createFromFormat('U', floor($now + $waitDuration)), true, $this->limit));
65+
} elseif ($availableTokens >= $tokens) {
6366
$window->add($tokens, $now);
6467

6568
$reservation = new Reservation($now, new RateLimit($window->getAvailableTokens($now), \DateTimeImmutable::createFromFormat('U', floor($now)), true, $this->limit));
6669
} else {
67-
$waitDuration = $window->calculateTimeForTokens(max(1, $tokens), $now);
70+
$waitDuration = $window->calculateTimeForTokens($tokens, $now);
6871

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

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Policy/SlidingWindowLimiter.php
+5-4Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,13 @@ public function consume(int $tokens = 1): RateLimit
6969
return new RateLimit($availableTokens, $window->getRetryAfter(), false, $this->limit);
7070
}
7171

72-
$window->add($tokens);
73-
74-
if (0 < $tokens) {
75-
$this->storage->save($window);
72+
if (0 === $tokens) {
73+
return new RateLimit($availableTokens, $availableTokens ? \DateTimeImmutable::createFromFormat('U.u', sprintf('%.6F', microtime(true))) : $window->getRetryAfter(), true, $this->limit);
7674
}
7775

76+
$window->add($tokens);
77+
$this->storage->save($window);
78+
7879
return new RateLimit($this->getAvailableTokens($window->getHitCount()), $window->getRetryAfter(), true, $this->limit);
7980
} finally {
8081
$this->lock?->release();

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Policy/TokenBucketLimiter.php
+11-2Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,20 @@ public function reserve(int $tokens = 1, float $maxTime = null): Reservation
6767
$now = microtime(true);
6868
$availableTokens = $bucket->getAvailableTokens($now);
6969

70-
if ($availableTokens >= max(1, $tokens)) {
70+
if ($availableTokens >= $tokens) {
7171
// tokens are now available, update bucket
7272
$bucket->setTokens($availableTokens - $tokens);
7373

74-
$reservation = new Reservation($now, new RateLimit($bucket->getAvailableTokens($now), \DateTimeImmutable::createFromFormat('U', floor($now)), true, $this->maxBurst));
74+
if (0 === $availableTokens) {
75+
// This means 0 tokens where consumed (discouraged in most cases).
76+
// Return the first time a new token is available
77+
$waitDuration = $this->rate->calculateTimeForTokens(1);
78+
$waitTime = \DateTimeImmutable::createFromFormat('U', floor($now + $waitDuration));
79+
} else {
80+
$waitTime = \DateTimeImmutable::createFromFormat('U', floor($now));
81+
}
82+
83+
$reservation = new Reservation($now, new RateLimit($bucket->getAvailableTokens($now), $waitTime, true, $this->maxBurst));
7584
} else {
7685
$remainingTokens = $tokens - $availableTokens;
7786
$waitDuration = $this->rate->calculateTimeForTokens($remainingTokens);

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Tests/Policy/FixedWindowLimiterTest.php
+14Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,21 @@ public function testPeekConsume()
123123
$rateLimit = $limiter->consume(0);
124124
$this->assertSame(10, $rateLimit->getLimit());
125125
$this->assertTrue($rateLimit->isAccepted());
126+
$this->assertEquals(
127+
\DateTimeImmutable::createFromFormat('U', (string) floor(microtime(true))),
128+
$rateLimit->getRetryAfter()
129+
);
126130
}
131+
132+
$limiter->consume();
133+
134+
$rateLimit = $limiter->consume(0);
135+
$this->assertEquals(0, $rateLimit->getRemainingTokens());
136+
$this->assertTrue($rateLimit->isAccepted());
137+
$this->assertEquals(
138+
\DateTimeImmutable::createFromFormat('U', (string) floor(microtime(true) + 60)),
139+
$rateLimit->getRetryAfter()
140+
);
127141
}
128142

129143
public static function provideConsumeOutsideInterval(): \Generator

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Tests/Policy/SlidingWindowLimiterTest.php
+16Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ protected function setUp(): void
3131

3232
ClockMock::register(InMemoryStorage::class);
3333
ClockMock::register(RateLimit::class);
34+
ClockMock::register(SlidingWindowLimiter::class);
3435
}
3536

3637
public function testConsume()
@@ -82,11 +83,26 @@ public function testPeekConsume()
8283

8384
$limiter->consume(9);
8485

86+
// peek by consuming 0 tokens twice (making sure peeking doesn't claim a token)
8587
for ($i = 0; $i < 2; ++$i) {
8688
$rateLimit = $limiter->consume(0);
8789
$this->assertTrue($rateLimit->isAccepted());
8890
$this->assertSame(10, $rateLimit->getLimit());
91+
$this->assertEquals(
92+
\DateTimeImmutable::createFromFormat('U.u', sprintf('%.6F', microtime(true))),
93+
$rateLimit->getRetryAfter()
94+
);
8995
}
96+
97+
$limiter->consume();
98+
99+
$rateLimit = $limiter->consume(0);
100+
$this->assertEquals(0, $rateLimit->getRemainingTokens());
101+
$this->assertTrue($rateLimit->isAccepted());
102+
$this->assertEquals(
103+
\DateTimeImmutable::createFromFormat('U.u', sprintf('%.6F', microtime(true) + 12)),
104+
$rateLimit->getRetryAfter()
105+
);
90106
}
91107

92108
private function createLimiter(): SlidingWindowLimiter

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/RateLimiter/Tests/Policy/TokenBucketLimiterTest.php
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,11 +134,26 @@ public function testPeekConsume()
134134

135135
$limiter->consume(9);
136136

137+
// peek by consuming 0 tokens twice (making sure peeking doesn't claim a token)
137138
for ($i = 0; $i < 2; ++$i) {
138139
$rateLimit = $limiter->consume(0);
139140
$this->assertTrue($rateLimit->isAccepted());
140141
$this->assertSame(10, $rateLimit->getLimit());
142+
$this->assertEquals(
143+
\DateTimeImmutable::createFromFormat('U', (string) floor(microtime(true))),
144+
$rateLimit->getRetryAfter()
145+
);
141146
}
147+
148+
$limiter->consume();
149+
150+
$rateLimit = $limiter->consume(0);
151+
$this->assertEquals(0, $rateLimit->getRemainingTokens());
152+
$this->assertTrue($rateLimit->isAccepted());
153+
$this->assertEquals(
154+
\DateTimeImmutable::createFromFormat('U', (string) floor(microtime(true) + 1)),
155+
$rateLimit->getRetryAfter()
156+
);
142157
}
143158

144159
public function testBucketRefilledWithStrictFrequency()

0 commit comments

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