8000 bug #53259 [RateLimit] Test and fix peeking behavior on rate limit po… · symfony/symfony@64f6ac2 · GitHub
[go: up one dir, main page]
Skip to content

Commit 64f6ac2

Browse 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 | ------------- | --- | Bra 10000 nch? | 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

File tree

6 files changed

+66
-8
lines changed

6 files changed

+66
-8
lines changed

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

+5-2
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

+5-4
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

+11-2
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

+14
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

+16
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

+15
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