[Cache] Prevent stampede at warmup using flock() #27604
Conversation
7c8dafa to
958f0cb
Compare
|
PR ready |
958f0cb to
0ac2777
Compare
|
Thank you @nicolas-grekas. |
…las-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Cache] Prevent stampede at warmup using flock() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Replaces #27028 This PR protects against cache stampede by wrapping the computation of items in a pool of locks. For each apps, there can be at most 20 concurrent processes that compute items at the same time and only one per cache-key. Commits ------- 0ac2777 [Cache] Prevent stampede at warmup using flock()
|
I spent a few hours reviewing most of your performance changes, I have been impressed by most of it (the probabilistic early expiration is a very nice feature to have, although I don't like the way you implemented it because it does very heavy magic relying a bit too much on PHP internals for my taste, but that's details, and I won't fight over details because you did a great job) although, reviewing this one, there are a few points worrying me:
Besides, I can see were it could cause trouble, a simple example from my mind (but I might be wrong so please correct me if there's something I missed) but what happens if I use a bundle that uses the new CacheInterface to store business cache items (not system cache here) let's say in a shared Redis instance, and I have a certain number of web heads with each of them their own local filesytem, wouldn't that mean that eventually it would trigger the LockRegistry usage, were in that case it seems highly unnecessary ? I had thought the last few hours about a couple of other uses cases were it could go wrong, or just be useless, but I don't remember them all. Don't take it wrong, what you did here is a good move toward stampede protection, I'm just very worried that the flock() usage is hardcoded, and there's no easy way to deactivate or implement another way than by decorating or overriding services. |
|
Moreover, as a side note, it seems that ArrayAdapter and NullAdapter don't need the GetTrait at all, they would function properly with a more straight foward implementation without the the stampede protection as they are meant to live into memory only for the request. |
|
Hi @pounard thanks for your comments, that's great to know some care and noticed :) Can you open an issue so we can discuss this? I'd be happy to further improve. |
|
@nicolas-grekas thanks for you for answering so fast ! I will open an issue with pleasure, I'll try to do a more structured write up to start with, I will within the week (tomorow or the day after). Thanks. |
Replaces #27028
This PR protects against cache stampede by wrapping the computation of items in a pool of locks.
For each apps, there can be at most 20 concurrent processes that compute items at the same time and only one per cache-key.