8000 bug #51666 [RateLimiter] CompoundLimiter was accepting requests even … · symfony/symfony@82b811d · GitHub
[go: up one dir, main page]
Skip to content

Commit 82b811d

Browse files
nicolas-grekasnicolas-grekas
committed
bug #51666 [RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens (10n)
This PR was merged into the 6.3 branch. Discussion ---------- [RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix 8000 ? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - CompoundLimiter is accepting requests when the limit was reached previously. When processing the limiters and the first one consumes exactly all the remaining tokens (remaining=0, accepted=true) and the next one already reached the limit previously (remaining=0, accepted=0) the $minimalRateLimit is considered the first one that will accept the request (even if it's not the most restrictive). For example: CompoundLimiter includes 2 limiters: - limiter 1 - remaining 2 tokens - limiter 2 - remaining 0 tokens After consuming 2 tokens each each limiter generates to limits: - `limiter1`->consume(2), generates a limit indicating `0` remaining tokens, **accepts** the request (it was last permitted) - `limiter2`->consume(2), generates a limit indicating `0` remaining tokens, **did not accept** the request (it did not have 2 tokens to satisfy the request) Because both of them have at this moment `0` remaining tokens, the minimum limit that is returned will be the limit from the `limiter1` . This means that the CompundLimiter will accept the request, even if the `limiter2` should be more restrictive. If we switch the order in the constructor, the request will be denied. The order should not matter. Commits ------- 65ce7f8 [RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens
2 parents 7610bc2 + 65ce7f8 commit 82b811d

File tree

2 files changed

+43
-9
lines changed

2 files changed

+43
-9
lines changed

‎src/Symfony/Component/RateLimiter/CompoundLimiter.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,11 @@ public function consume(int $tokens = 1): RateLimit
4242
foreach ($this->limiters as $limiter) {
4343
$rateLimit = $limiter->consume($tokens);
4444

45-
if (null === $minimalRateLimit || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()) {
45+
if (
46+
null === $minimalRateLimit
47+
|| $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()
48+
|| ($minimalRateLimit->isAccepted() && !$rateLimit->isAccepted())
49+
) {
4650
$minimalRateLimit = $rateLimit;
4751
}
4852
}

‎src/Symfony/Component/RateLimiter/Tests/CompoundLimiterTest.php

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,19 +36,49 @@ public function testConsume()
3636
{
3737
$limiter1 = $this->createLimiter(4, new \DateInterval('PT1S'));
3838
$limiter2 = $this->createLimiter(8, new \DateInterval('PT10S'));
39-
$limiter3 = $this->createLimiter(12, new \DateInterval('PT30S'));
39+
$limiter3 = $this->createLimiter(16, new \DateInterval('PT30S'));
4040
$limiter = new CompoundLimiter([$limiter1, $limiter2, $limiter3]);
4141

42-
$this->assertEquals(0, $limiter->consume(4)->getRemainingTokens(), 'Limiter 1 reached the limit');
42+
$rateLimit = $limiter->consume(4);
43+
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
44+
$this->assertTrue($rateLimit->isAccepted(), 'All limiters accept (exact limit on limiter 1)');
45+
46+
$rateLimit = $limiter->consume(1);
47+
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 1 reached the limit');
48+
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 1 did not accept limit');
49+
4350
sleep(1); // reset limiter1's window
44-
$this->assertTrue($limiter->consume(3)->isAccepted());
4551

46-
$this->assertEquals(0, $limiter->consume()->getRemainingTokens(), 'Limiter 2 has no remaining tokens left');
47-
sleep(10); // reset limiter2's window
48-
$this->assertTrue($limiter->consume(3)->isAccepted());
52+
$rateLimit = $limiter->consume(3);
53+
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 consumed exactly the remaining tokens');
54+
$this->assertTrue($rateLimit->isAccepted(), 'All accept the request (exact limit on limiter 2)');
55+
56+
$rateLimit = $limiter->consume(1);
57+
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 2 had remaining tokens left');
58+
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 did not accept the request');
59+
60+
sleep(1); // reset limiter1's window again, to make sure that the limiter2 overrides limiter1
61+
62+
// make sure to consume all allowed by limiter1, limiter2 already had 0 remaining
63+
$rateLimit = $limiter->consume(4);
64+
$this->assertEquals(
65+
0,
66+
$rateLimit->getRemainingTokens(),
67+
'Limiter 1 consumed the remaining tokens (accept), Limiter 2 did not have any remaining (not accept)'
68+
);
69+
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 2 reached the limit already');
70+
71+
sleep(10); // reset limiter2's window (also limiter1)
72+
73+
$rateLimit = $limiter->consume(3);
74+
$this->assertEquals(0, $rateLimit->getRemainingTokens(), 'Limiter 3 had exactly 3 tokens (accept)');
75+
$this->assertTrue($rateLimit->isAccepted());
76+
77+
$rateLimit = $limiter->consume(1);
78+
$this->assertFalse($rateLimit->isAccepted(), 'Limiter 3 reached the limit previously');
79+
80+
sleep(30); // reset limiter3's window (also limiter1 and limiter2)
4981

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

0 commit comments

Comments
 (0)
0