[RateLimiter] Always store SlidingWindows with an expiration set #44996

Seldaek · Jan 12, 2022

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Refs #44995
License	MIT
Doc PR	symfony/symfony-docs#...

See #44995 for more details. The short story is that this code causes the cache item to be overwritten without TTL in

symfony/src/Symfony/Component/RateLimiter/Storage/CacheStorage.php

Lines 31 to 37 in 6bffe41

    
           $cacheItem = $this->pool->getItem(sha1($limiterState->getId())); 
        
           $cacheItem->set($limiterState); 
        
           if (null !== ($expireAfter = $limiterState->getExpirationTime())) { 
        
               $cacheItem->expiresAfter($expireAfter); 
        
           } 
        
           $this->pool->save($cacheItem);

So our redis DB is filling up with rate limiter data, which is not the expected outcome. I couldn't really figure out why the logic was the way it was so it seems safe for me to remove this, but maybe I missed something.

As a related side note, implementing __serialize/__unserialize in SlidingWindow and co would be good as it would reduce the storage requirements in the cache from 400 bytes to 100 bytes per rate limiter stored. I'm happy to help with that just let me know, I am not sure if there was a good reason not to implement this or if storage size was simply not a consideration. I see __sleep is implemented but that does not seem to help here, not sure what the purpose is/was.

stof · Jan 12, 2022

I see __sleep is implemented but that does not seem to help here, not sure what the purpose is/was.

I think the purpose was precisely to reduce the storage size

Seldaek · Jan 12, 2022

It does not seem to have any effect though, so I think it was mostly there to exclude the cached property: https://3v4l.org/nHuAQ

FYI here is the full output with igbinary:

string 'O:3:"Noo":2:{s:6:"�*�bar";s:7:"sdfpods";s:7:"�*�bloo";s:8:"blkdsjds";}' (length=70)
string 'O:3:"Foo":2:{s:6:"�*�bar";s:7:"sdfpods";s:7:"�*�bloo";s:8:"blkdsjds";}' (length=70)
string 'O:3:"Bar":2:{i:0;s:7:"sdfpods";i:1;s:8:"blkdsjds";}' (length=51)
string '���STXETBETXNooDC4STXDC1ACK�*�barDC1BELsdfpodsDC1BEL�*�blooDC1BSblkdsjds' (length=47)
string '���STXETBETXFooDC4STXDC1ACK�*�barDC1BELsdfpodsDC1BEL�*�blooDC1BSblkdsjds' (length=47)
string '���STXETBETXBarDC4STXACK�DC1BELsdfpodsACKSOHDC1BSblkdsjds' (length=34)

You can see it is quite a bit shorter with __serialize, but it's much more with the RateLimiter classes because somehow the serialized output includes the property names with full class name every time, it's extremely wasteful:

One example from my prod redis server: \x00\x00\x00\x02\x172Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x14\x05\x116\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00id\x11\x1clogin_per_ip-000.000.214.194\x11<\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00hitCount\x06\x01\x11E\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00intervalInSeconds\x06<\x11I\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00hitCountForLastWindow\x06\x00\x11?\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00windowEndAt\na\xde\xc1\xfa

Seldaek · Jan 12, 2022

In any case, I think we need @wouterj or @Nyholm to shine some light on this :)

Nyholm · Feb 4, 2022

Oh no, you are very correct. This is indeed a bug. I’m not sure about your solution. I think there is a good reason why we didn’t set a new expectation time, Ie we don’t want to move the window. (Yes, it is “sliding” but we need a clear start and end of each window which is defined by the cache item)

I need to give this a second look asap to confirm. (I’m currently traveling and my computer broke down)

Seldaek · Mar 1, 2022

@Nyholm ping here? I'd be happy to get this resolved at some point, as our redis keeps filling up right now :)

Thanks to the bug right now, the cache item doesn't expire ever if you hit more than once.

So either the way I see it, either:

you hit once, then it disappears after 2x $intervalsInSeconds (SlidingWindow's interval), then you come back and you hit and it's a fresh SlidingWindow
you hit it twice, then if you hit a third time it is reloaded from cache, you are still in the active window and so things work using that existing one
you hit it twice, so it stays in the cache, but you hit a third time after the sliding window expired, then I assume createFromPreviousWindow is called to create a new one, which is correct behavior too, but if it was cleared from the cache because you come later than 2x $intervalsInSeconds it would simply create a blank one and that'd be correct too.

So the fix seems correct to me (it would only change the last bit of the last bullet point above), but maybe I misunderstood something of course.

Seldaek · Mar 16, 2022

Rebased/retargetted to 5.4 as 5.3 is now EOL

Nyholm · Mar 17, 2022

Please let me delay this week too. I have an open source session planned on Sunday. If I fail to come back then just ignore me.

Sorry for being unavailable.

src/Symfony/Component/RateLimiter/Policy/SlidingWindow.php

fabpot · Mar 26, 2022

Any news here?

wouterj

Thanks!

It does not seem to have any effect though, so I think it was mostly there to exclude the cached property

Indeed, I don't think storage size was considered here.

src/Symfony/Component/RateLimiter/Tests/Policy/SlidingWindowTest.php

src/Symfony/Component/RateLimiter/Policy/SlidingWindow.php

fabpot · Mar 29, 2022

Thank you @Seldaek.

Seldaek · Mar 29, 2022

I tested the patch with the latest changes from @nicolas-grekas and the good news is it does work well doing:

"SETEX" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6" "119" "\x00\x00\x00\x02\x172Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x14\x01\x117\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00<login_per_ip_username-sdfsdf@fdjk-127.0.0.1\x0cA\xd8\x90\xb3\x9e\x10\x0es"

instead of:

"SET" "lIoZODSNyI:921c2aa99364ab8bbb0e4525c4297aa71c61d2c6" "\x00\x00\x00\x02\x172Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x14\x05\x116\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00id\x11\x16login_per_ip-127.0.0.1\x11<\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00hitCount\x06\x02\x11E\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00intervalInSeconds\x06<\x11I\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00hitCountForLastWindow\x06\x00\x11?\x00Symfony\\Component\\RateLimiter\\Policy\\SlidingWindow\x00windowEndAt\x0cA\xd8\x90\xb3#^z\xcc"

So much more efficient storage, and SETEX instead of SET 👍🏻

Bad news is the migration path is a bit broken (like you get a fatal error..) if it reloads existing data which was serialized with __sleep and tries to unserialize using the new __unserialize. I'll send a follow-up PR.

wouterj · Mar 29, 2022

thanks for the quick checking in a real app :)

Seldaek · Mar 29, 2022

Here #45876

…che (Seldaek) This PR was merged into the 5.4 branch. Discussion ---------- Add BC layer to handle old objects already present in cache | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | Refs #44996 | License | MIT | Doc PR | symfony/symfony-docs#...  When reloading an old RateLimiter instance from cache with the new code I did get an exception because windowEndAt was a string and getHitCount does windowEndAt - intervalInSeconds, and string - int fails. The reason is the new code assigns the id to windowEndAt via: ``` $pack = key($data); $this->windowEndAt = $data[$pack]; ``` I dumped $this and $data in __unserialize just FYI and you get this: ``` object(Symfony\Component\RateLimiter\Policy\SlidingWindow)[959] private 'id' => null private 'hitCount' => int 0 private 'hitCountForLastWindow' => int 0 private 'intervalInSeconds' => null private 'windowEndAt' => null ``` ``` array (size=5) '�Symfony\Component\RateLimiter\Policy\SlidingWindow�id' => string 'login_per_ip-127.0.0.1' (length=22) '�Symfony\Component\RateLimiter\Policy\SlidingWindow�hitCount' => int 2 '�Symfony\Component\RateLimiter\Policy\SlidingWindow�intervalInSeconds' => int 60 '�Symfony\Component\RateLimiter\Policy\SlidingWindow�hitCountForLastWindow' => int 0 '�Symfony\Component\RateLimiter\Policy\SlidingWindow�windowEndAt' => float 1648544909.4762 ``` I only tested the code in real conditions for SlidingWindow, but I do believe/hope the same applies for the other two policies. Commits ------- f039ee6 Add BC layer to handle old objects already present in cache

Seldaek · Apr 6, 2022

Confirmed to work in prod now with 6.0.7 🥳 Thanks again everyone.

carsonbot added Status: Needs Review Bug RateLimiter labels Jan 12, 2022

carsonbot added this to the 5.3 milestone Jan 12, 2022

Seldaek mentioned this pull request Jan 12, 2022

[Cache] Cached item loses TTL information when retrieved and stored again #44995

Closed

Seldaek changed the base branch from 5.3 to 5.4 March 16, 2022 15:40

Seldaek force-pushed the patch-21 branch from ab62542 to bbd6f76 Compare March 16, 2022 15:41

chalasr approved these changes Mar 17, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 17, 2022

nicolas-grekas reviewed Mar 17, 2022

View reviewed changes

src/Symfony/Component/RateLimiter/Policy/SlidingWindow.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Mar 17, 2022

View reviewed changes

src/Symfony/Component/RateLimiter/Policy/SlidingWindow.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Mar 17, 2022

View reviewed changes

src/Symfony/Component/RateLimiter/Policy/SlidingWindow.php Outdated Show resolved Hide resolved

nicolas-grekas modified the milestones: 5.3, 5.4 Mar 28, 2022

nicolas-grekas force-pushed the patch-21 branch from bbd6f76 to 3a72990 Compare March 28, 2022 10:54

wouterj approved these changes Mar 28, 2022

View reviewed changes

src/Symfony/Component/RateLimiter/Tests/Policy/SlidingWindowTest.php Outdated Show resolved Hide resolved

Seldaek commented Mar 28, 2022

View reviewed changes

src/Symfony/Component/RateLimiter/Policy/SlidingWindow.php Show resolved Hide resolved

nicolas-grekas force-pushed the patch-21 branch 2 times, most recently from 5d96d1d to 5336906 Compare March 28, 2022 15:12

[RateLimiter] Always store SlidingWindows with an expiration set

54cacfe

nicolas-grekas force-pushed the patch-21 branch from 5336906 to 54cacfe Compare March 28, 2022 15:19

nicolas-grekas approved these changes Mar 28, 2022

View reviewed changes

fabpot merged commit 5dd5dd2 into symfony:5.4 Mar 29, 2022

Seldaek deleted the patch-21 branch March 29, 2022 09:05

Seldaek mentioned this pull request Mar 29, 2022

Add BC layer to handle old objects already present in cache #45876

Merged

This was referenced Apr 2, 2022

Release v5.4.7 #45915

Merged

Release v6.0.7 #45916

Merged

mbrodala mentioned this pull request Dec 6, 2023

Cannot use interval > 27263 seconds brotkrueml/typo3-form-rate-limit#5

Closed

	$cacheItem = $this->pool->getItem(sha1($limiterState->getId()));
	$cacheItem->set($limiterState);
	if (null !== ($expireAfter = $limiterState->getExpirationTime())) {
	$cacheItem->expiresAfter($expireAfter);
	}

	$this->pool->save($cacheItem);

Search code, repositories, users, issues, pull requests...

Uh oh!

[RateLimiter] Always store SlidingWindows with an expiration set #44996

[RateLimiter] Always store SlidingWindows with an expiration set #44996

Uh oh!

Conversation

Seldaek commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Jan 12, 2022

Uh oh!

Seldaek commented Jan 12, 2022

Uh oh!

Seldaek commented Jan 12, 2022

Uh oh!

Nyholm commented Feb 4, 2022

Uh oh!

Seldaek commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Seldaek commented Mar 16, 2022

Uh oh!

Nyholm commented Mar 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabpot commented Mar 26, 2022

Uh oh!

wouterj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fabpot commented Mar 29, 2022

Uh oh!

Seldaek commented Mar 29, 2022

Uh oh!

wouterj commented Mar 29, 2022

Uh oh!

Seldaek commented Mar 29, 2022

Uh oh!

Seldaek commented Apr 6, 2022

Uh oh!

Uh oh!

Seldaek commented Jan 12, 2022 •

edited

Loading

Seldaek commented Mar 1, 2022 •

edited

Loading