8000 [RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens by 10n · Pull Request #51666 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens #51666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 10, 2023
Merged

[RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens #51666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Symfony/Component/RateLimiter/CompoundLimiter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ public function consume(int $tokens = 1): RateLimit
foreach ($this->limiters as $limiter) {
$rateLimit = $limiter->consume($tokens);

if (null === $minimalRateLimit || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()) {
if (
null === $minimalRateLimit
|| $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()
|| ($minimalRateLimit->isAccepted() && !$rateLimit->isAccepted())
) {
$minimalRateLimit = $rateLimit;
}
}
Expand Down
46 changes: 38 additions & 8 deletions src/Symfony/Component/RateLimiter/Tests/CompoundLimiterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,49 @@ public function testConsume()
{
$limiter1 = $this->createLimiter(4, new \DateInterval('PT1S'));
$limiter2 = $this->createLimiter(8, new \DateInterval('PT10S'));
$limiter3 = $this->createLimiter(12, new \DateInterval('PT30S'));
$limiter3 = $this->createLimiter(16, new \DateInterval('PT30S'));
$limiter = new CompoundLimiter([$limiter1, $limiter2, $limiter3]);

$this->assertEquals(0, $limiter->consume(4)->getRemainingTokens(), 'Limiter 1 reached the limit');
$rateLimit = $limiter->consume(4);
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
$this->assertTrue($rateLimit->isAccepted(), 'All limiters accept (exact limit on limiter 1)');

$rateLimit = $limiter->consume(1);
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 1 did not accept limit');

sleep(1); // reset limiter1's window
$this->assertTrue($limiter->consume(3)->isAccepted());

$this->assertEquals(0, $limiter->consume()->getRemainingTokens(), 'Limiter 2 has no remaining tokens left');
sleep(10); // reset limiter2's window
$this->assertTrue($limiter->consume(3)->isAccepted());
$rateLimit = $limiter->consume(3);
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 consumed exactly the remaining tokens');
$this->assertTrue($rateLimit->isAccepted(), 'All accept the request (exact limit on limiter 2)');

$rateLimit = $limiter->consume(1);
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 had remaining tokens left');
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 did not accept the request');

sleep(1); // reset limiter1's window again, to make sure that the limiter2 overrides limiter1

// make sure to consume all allowed by limiter1, limiter2 already had 0 remaining
$rateLimit = $limiter->consume(4);
$this->assertEquals(
0,
$rateLimit->getRemainingTokens(),
'Limiter 1 consumed the remaining tokens (accept), Limiter 2 did not have any remaining (not accept)'
);
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 reached the limit already');

sleep(10); // reset limiter2's window (also limiter1)

$rateLimit = $limiter->consume(3);
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 3 had exactly 3 tokens (accept)');
$this->assertTrue($rateLimit->isAccepted());

$rateLimit = $limiter->consume(1);
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 3 reached the limit previously');

sleep(30); // reset limiter3's window (also limiter1 and limiter2)

$this->assertEquals(0, $limiter->consume()->getRemainingTokens(), 'Limiter 3 reached the limit');
sleep(20); // reset limiter3's window
$this->assertTrue($limiter->consume()->isAccepted());
}

Expand Down
0