Dynamic Page Cache (DPC) is unable to retrieve a previously stored response from cache, due to AmpEventSubscriber::onView() setting ?_wrapper_format=html at Kernel::VIEW time.
This is due to DPC first resolving url.query_args:_wrapper_format at Kernel::REQUEST time (when it tries to load the cached response, before Kernel::VIEW), then relying on resolving url.query_args:_wrapper_format at Kernel::RESPONSE time (when it stores the response in the cache, after Kernel::VIEW). Due to AmpEventSubscriber changing the value in-between the two times DPC resolves it, the cid used by DPC will not be the same when loading vs. storing in the cache, which will cause MISSes every time.
This has only been observed on node pages.
- Install Drupal 8.9.x
- Enable amp (3.5)
- Disable page_cache
- Create a test node
- Load the test node detail page (1st time): X-Drupal-Dynamic-Cache: MISS
- Load the test node detail page (2nd time): X-Drupal-Dynamic-Cache: MISS (wrong!)
Just remove the if branch that set _wrapper_format=html for non-AMP pages (AmpEventSubscriber::onView).
- Make the bug reproducible with a Functional test.
- Fix the bug.
- Review the fix.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | amp-dynamic-page-cache-fix-3207332-15.patch | 2.43 KB | jedihe |
| #9 | 3207332-amp-dynamic-page-cache-interference-9--test-only.patch | 1.43 KB | jedihe |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
jedihe commentedLet's start by properly detecting the bug, as well as explore the first idea for a fix.
The issue fork has the same code changes as the full patch.
Comment #3
jedihe commentedLooks like a typo in the test class name prevented the test runner from finding the new test.
Comment #4
jedihe commentedAlright! the test works to detect the bug :).
Judging from tests, the attempted fix works; let's try removing the if branch that caused the problem in the first place and ask for review.
Comment #6
jedihe commentedComment #7
jedihe commentedComment #8
jedihe commentedDon't know what's preventing the testbot to run the new test in the MR. I'll just give the testbot patches, with the latest changes in the MR.
Comment #9
jedihe commentedGood! and now for the test-only patch.
Comment #10
jedihe commentedComment #12
jedihe commentedGiven #9 (1 fail, for the new test) is the test-only version of the MR @e5a05e1d (41 passes, 1 more than 3.x-dev), this is ready for review.
Comment #13
damienmckennaThe MR/patch looks good, but I'll leave someone else to RTBC it as I haven't tested it yet.
Comment #14
berdirDid run into this as well.
This doesn't only affect dynamic page cache but also active menu links, or more specifically \Drupal\Core\EventSubscriber\ActiveLinkResponseFilter::setLinkActiveClass. That will end up with that wrapper format in $query and will then not mark links as active.
I don't understand why this exists in the first place, but can confirm that this causes problems and should not be done.
Comment #15
jedihe commentedJust attaching the patch for the MR @e5a05e1d. test-only patch is the same as #9.
Comment #17
berdirLooks like HEAD passed against 9.2, is this causing a regression somehow?
Comment #18
jedihe commentedHad a look into this and found something odd: locally, I can get 8.x-3.x to trigger various failures in the test suite, but the testbot does not.
For now, I opened #3232376: Test failures on PHP 7.3, D9.2 and committed two trivial fixes, but couldn't yet understand what causes the errors in #15. I think there is some stabilization work pending in the test-suite, which is the actual cause for those errors.