Problem/Motivation

PageCache::getCacheId() is called during get() and then again during set(). Because it calls functions on $request, this means it can return a different $cid during set() than it will look up during subsequent get()s. For example, a request to a URL that is only available in JSON format could have $request->getRequestFormat() return 'html' (the default return value of that function) during page cache lookup and 'json' during page cache writing (after routing has determined that that is the default format of that URL). This results in unnecessary cache misses for requests to non-HTML resources.

Proposed resolution

Once a cache ID is determined during lookup, use the same one when writing.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 page-cache-consistency-14.patch8.18 KBeffulgentsia
#15 interdiff.txt2.15 KBeffulgentsia
#15 page-cache-consistency-15.patch7.7 KBeffulgentsia
#14 page-cache-consistency-14.patch8.18 KBwim leers
#14 interdiff.txt1.46 KBwim leers
#11 interdiff.txt1.87 KBeffulgentsia
#11 page-cache-consistency-11.patch7.64 KBeffulgentsia
#9 page-cache-consistency-9.patch5.59 KBwim leers
#9 interdiff.txt983 byteswim leers
#6 page-cache-consistency-6.patch4.76 KBwim leers
#4 page-cache-consistency.patch2.77 KBeffulgentsia
#3 page-cache-consistency-test-only.patch1.26 KBeffulgentsia

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

effulgentsia commented
Title: PageCache::getCacheId() should return the same cache ID during get and set » PageCache::getCacheId() sometimes uses different cache IDs during get and set, causing unnecessary cache misses for some requests
effulgentsia’s picture

effulgentsia commented
StatusFileSize
new page-cache-consistency-test-only.patch1.26 KB

Here's a test that demonstrates the failure.

effulgentsia’s picture

effulgentsia commented
Status: Active » Needs review
StatusFileSize
new page-cache-consistency.patch2.77 KB

Here's the fix.

Status: Needs review » Needs work

The last submitted patch, 4: page-cache-consistency.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Issue tags: +D8 cacheability
StatusFileSize
new page-cache-consistency-6.patch4.76 KB

The last submitted patch, 3: page-cache-consistency-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: page-cache-consistency-6.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt983 bytes
new page-cache-consistency-9.patch5.59 KB

Status: Needs review » Needs work

The last submitted patch, 9: page-cache-consistency-9.patch, failed testing. View results

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
StatusFileSize
new page-cache-consistency-11.patch7.64 KB
new interdiff.txt1.87 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +API-First Initiative
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -339,11 +339,22 @@ protected function set(Request $request, Response $response, $expire, array $tag
+    // will be the value during lookups for subsequent requests.
+    $cid_attribute = '_page_cache_id';
+    if (!$request->attributes->has($cid_attribute)) {
+      $cid_parts = [

Why can't this be a protected property on the page cache middleware itself?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1.46 KB
new page-cache-consistency-14.patch8.18 KB

The PageCache middleware only handles the master request, of which there can only be one, so … you're right!

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new page-cache-consistency-15.patch7.7 KB
new interdiff.txt2.15 KB

Let's do it this way then. Same functionality, just a style difference.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -346,20 +347,11 @@
-        $request->getRequestFormat(NULL),
...
+      $request->getRequestFormat(),

You've now introduced a functional change that should cause failures.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new page-cache-consistency-14.patch8.18 KB

Re #16, oops.

And regardless of that, I changed my mind and think #14 is better after all, so reuploading that one.

The last submitted patch, 15: page-cache-consistency-15.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Hah! 😀

  • catch committed 49ef12b on 8.7.x 
    Issue #3029373 by effulgentsia, Wim Leers, catch: PageCache::getCacheId...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 49ef12b and pushed to 8.7.x. Thanks!

Status: Fixed » Closed (fixed)

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

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented

I know this isn't a normal core use case but this makes making two requests in one Drupal bootstrap impossible as the CID is the same for both requests. Will decorate the service for Tome to workaround this.

effulgentsia’s picture

effulgentsia commented

I was wondering if it was possible to have multiple master requests in the same Drupal bootstrap. I guess #23 means yes. Note to self: look at Tome to see how it does it.