Skip to content

Commit 64f6ac2

Browse files
nicolas-grekasnicolas-grekas
committed
bug symfony#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 symfony#52835 | License | MIT Although heavily discouraged for user-land code, we've implemented peeking behavior for rate limiting in 6.2 with symfony#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 symfony#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 symfony#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

File tree

6 files changed

+66
-8
lines changed

6 files changed

+66
-8
lines changed

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

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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‎

Lines 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)