Discovered in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). The redirecting cache item would be created in the default cache bin (render), the actual render cache item would be stored in the specified cache bin. This the of course led to the cache redirect cache item not being found, and thus resulting in a cache miss on every request.

CommentFileSizeAuthor
#7 cache_redirect_bin-2493047-7-test-only.patch5.64 KBwim leers
#7 interdiff.txt4.36 KBwim leers
#7 cache_redirect_bin-2493047-7.patch6.67 KBwim leers
#1 cache_redirect_bin-2493047-1.patch2.71 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new cache_redirect_bin-2493047-1.patch2.71 KB
fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs tests, +Novice

RTBC, this tests via unit tests that the 'bin' is set to 'render' in the redirect cache item, hence enough test coverage exists.

fabianx’s picture

fabianx commented
Issue tags: -Needs tests, -Novice

No, it has enough tests ... - as said already.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Thanks! More cache fixing goodness.

I'm not clear on why this doesn't need tests though. @Fabianx, how is there enough test coverage if there was a cache miss on every request under... the circumstance that is not quite clear to me... and nothing was failing in HEAD? That sounds like we definitely need more test coverage; the unit tests definitely aren't enough with a bug of that magnitude? Refer to https://www.drupal.org/core-gates#testing

So let's add test coverage, or at least a really thorough justification of how it is we don't need it...

fabianx’s picture

fabianx commented
Related issues: +#2463581: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age

Related #2463581: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age - maybe we want to incorporate that one in here?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I'm not sure about #2463581: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age. I'm posting a comment there that explains what I still find unclear. So let's keep that as a separate issue.

EDIT: see #2463581-3: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new cache_redirect_bin-2493047-7.patch6.67 KB
new interdiff.txt4.36 KB
new cache_redirect_bin-2493047-7-test-only.patch5.64 KB
fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC, looks great, thought test-bot will send this to NW as test-only patch was attached last ...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: cache_redirect_bin-2493047-7-test-only.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

RTBC per #8.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed d2ebb7a and pushed to 8.0.x. Thanks!

  • alexpott committed d2ebb7a on 8.0.x 
    Issue #2493047 by Wim Leers: Cache redirects should be stored in the...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: » #2493047: Cache redirects should be stored in the same cache bin
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: #2493047: Cache redirects should be stored in the same cache bin » #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.