#2429257: Bubble cache contexts introduced bubbling of cache contexts. And to do that, it introduced the (internal implementation detail) concept of "cache redirects".
This has worked great so far. However… as soon as you start caching responses that contain the results of cache redirect, you need to ensure that if the cache redirect is updated (i.e. an additional cache context is discovered), then the cached response needs to be invalidated.
It is not actually necessary when the the redirect is rendered into its final markup, because that final markup cannot (and should not) change, even after additional cache contexts bubble. (If that were the case, it'd be non-deterministic.) Which is why this hasn't been a problem yet.
But, as soon as you deal with placeholders (in the sense of #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache), i.e. a cache redirect that is rendered as a placeholder, then it becomes a different story. [more explanation needed]
From the patch:
+ // Create a redirect cache tag, which is used for two purposes:
+ // 1. invalidate cached supertrees (otherwise redirected render cache
+ // items may continue to not be automatically placeholdered, because a
+ // high-cardinality cache context was not yet known originally)
+ // 2. invalidate cache items that become obsolete when the cache contexts
+ // stored in the cache redirect are discovered to be incomplete.
So, this helps with keeping the number of cache items lower (2), but much more importantly, it is necessary for correctness: in case a redirect changes (i.e. additional cache contexts are bubbled), then something that is not yet placeholdered because only the ['user.roles'] cache context was bubbled, may suddenly need to become placeholdered, because now the ['session'] cache context is also bubbled. Without a cache tag to invalidate e.g. a SmartCache-cached page that contains the redirected, but non-placeholdered output that varies by user, that would not start showing a placeholder as it should.
Review.
None.
None.
None.
| Issue category | Bug because without this patch, it is posible that responses remain cached even when their contents should've changed. |
|---|---|
| Issue priority | Major because does not need to block release. |
| Prioritized changes | The main goal of this issue is performance/cacheability and their correctness. |
| Disruption | Only minimally disruptive for some contrib modules that have excellent test coverage: if they happen to test responses that go through the render system and contain render cache redirects, and they assert the response's cache tags, they need to be updated to account for an additional cache tag being present. |
Comments
Comment #2
wim leersComment #3
wim leersI'm not actually 100% certain we need this.
I was working on a concrete example that explains this. But then got stuck. Because I didn't see the problem anymore, i.e. the reason to do this.
This is blocked on feedback from @Fabianx.
Comment #5
lauriiiAssigning to Fabianx
Comment #6
xjmAlso deferring triage on @Fabianx's feedback.
Comment #7
fabianx commentedUnless someone can come up with a use case when this breaks, we can close this down I think.
I was not able to find any use case, though let me think again:
- We have a page it contains a 'block'
- The block changes, the page is expired
- We have a page it contains a 'block', it is placeholdered
- The block changes, the page is not expired as it never contained the block, but just a reference to it.
If however the block placement changes, then the page is expired and the placeholder is no longer reachable and hence expired, too indirectly.
Hence => no problem.
Comment #8
xjm