Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 #9560

adrian-martinez-interactiv4 · 2017-05-09T11:50:32Z

Description
This pull request expects to fix layout cache corruption due to async pagecache requests, when the async request generates a layout cacheId based on same handles as a cms page, but without loading the cms page associated, so cms page related layout updates get lost. Further explanation on issues #8554 and #9050. The testing scenario may sound weird, but it actually happens due to layout cache expiration, so layout cache expires but full page cache is still valid.

Fixed Issues

PageCache: async rendering of blocks can corrupt layout cache #8554: PageCache: async rendering of blocks can corrupt layout cache
Randomly getting an empty <body> that gets cached #9050: Randomly getting an empty body that gets cached

Manual testing scenarios

Use the cms home page to add a block via Layout Update XML. (I've also experienced issued with page layout setting not being used).
Ensure layout and full page cache are enabled, at least this two.
Visit cms page on frontend, so layout cache and full page cache are generated.
Clean layout cache: bin/magento cache:clean layout
curl -gI "http://local.mg2develop.com/page_cache/block/render/?ajax=1&blocks=[%22header.login%22]&handles=[%22default%22%2C%22cms_index_index%22%2C%22cms_page_view%22%2C%22cms_index_index_id_home%22]&originalRequest=a&version="
Clean only full page cache: bin/magento cache:clean full_page

Adapted Tests
\Magento\PageCache\Test\Unit\Controller\Block\RenderTest::testExecute
\Magento\PageCache\Test\Unit\Controller\Block\EsiTest::testExecute

Expected Result
Page rendered correctly, as the first time.

Actual Result
Depending on included content, full blank page may be returned, but at least, it fails to apply page layout update handles, so block added via layout update is not shown.

adragus-inviqa · 2017-05-09T15:37:47Z

Good catch. A couple of questions:

How is that additional_page_cache_handle handle fixing the issue? Can you expand on it a bit? Just for my knowledge.
Also, why not a constant?

adrian-martinez-interactiv4 · 2017-05-10T01:36:39Z

For sure, @adragus-inviqa, I'll try to detail my answer as much as I can.

About your first question: How is that additional_page_cache_handle handle fixing the issue? Can you expand on it a bit? Just for my knowledge.

For the example, we are using page custom layout update to insert a cms block into content container:

<body>
    <referenceContainer name="content">
        <block class="Magento\Cms\Block\Widget\Block" name="721858d09ed28df0b2b510ff93dfdec3"
               template="widget/static_block/default.phtml">
            <action method="setData">
                <argument name="name" xsi:type="string">block_id</argument>
                <argument name="value" xsi:type="string">1</argument>
            </action>
        </block>
    </referenceContainer>
</body>

First of all, layout cache does not only contain merged xml files; it saves separately:

Merged xml files : lib/internal/Magento/Framework/View/Model/Layout/Merge.php:443
Page layout info : lib/internal/Magento/Framework/View/Model/Layout/Merge.php:444
Page structure (for generating elements) : lib/internal/Magento/Framework/View/Layout.php:333

When layout data is saved into cache, it needs to generate a cache id. As it's considered that a unique set of handles should produce the same result, layout handles are used to generate an unique id for that set of handles. This can be checked here: \Magento\Framework\View\Model\Layout\Merge::getCacheId

    public function getCacheId()
    {
        return $this->generateCacheId(md5(implode('|', $this->getHandles())));
    }

This method is used by the three layout cache types explained above. Let's start erasing layout and full page cache.

When you are requesting a cms page, and full page cache is not hit, \Magento\Cms\Helper\Page::prepareResultPage method is called. This method is responsible of applying custom page layout handle and custom page layout updates for the current request. At the end of that method we find this:
$contentHeadingBlock = $resultPage->getLayout()->getBlock('page_content_heading');

This forces to build the layout in three steps, as found in \Magento\Framework\View\Layout\Builder::build method:

$this->loadLayoutUpdates();
$this->generateLayoutXml();
$this->generateLayoutBlocks();

The last two save data in layout cache; generateLayoutXml() method saves merged xml (including custom layout updates!) and page layout info, and generateLayoutBlocks() save the page structure.

Page gets rendered ok, cached data is ok, and output result is cached in full page cache. So far so good.

At this point, with layout and full page content cached, let's say your layout cache gets invalidated, because of its expiry date, or another reason, but your full page cache for that page is still valid. If you have a private content block in that cms page, an ajax call to /page_cache/block/render/ url is performed, containing the same layout handles that were used in normal load of the page (see example in PR description). Let's see what happens from here.

Since this ajax call uses the layout in the standard way to get blocks and render them, layout is rebuilt with specified handles. Previous steps building the layout are reproduced again, but without initializing the involved cms page, so its custom layout updates are not loaded. But as the layout handles are the same, the same cache id is generated, and merged xml and page structure get cached without the page custom updates. Now we have a cache version, with the same key, and diferent content than expected.

Now, let's clean only full page cache. You may think this is a little bit tricky scenario, so let's suppose full page cache expires at a different point in time that layout cache. Let's reload the page.

Full page cache misses, so, as said before, \Magento\Cms\Helper\Page::prepareResultPage method is called, adding proper page custom layout updates. Surprisingly, even with wrong merged xml files in layout cache (not including page custom layout), the resulting merged xml for the current request is ok, but block added via layout update is not showing up. What's happening?

The answer is in the step 3 of layout build, when layout tries to generate the blocks, \Magento\Framework\View\Layout::generateElements is called, and we find this piece of code here:

public function generateElements()
    {
        \Magento\Framework\Profiler::start(__CLASS__ . '::' . __METHOD__);
        $cacheId = 'structure_' . $this->getUpdate()->getCacheId();
        $result = $this->cache->load($cacheId);
        if ($result) {
            $data = $this->serializer->unserialize($result);
            $this->getReaderContext()->getPageConfigStructure()->populateWithArray($data['pageConfigStructure']);
            $this->getReaderContext()->getScheduledStructure()->populateWithArray($data['scheduledStructure']);
        } else {
            (...)
            $this->cache->save($this->serializer->serialize($data), $cacheId, $this->getUpdate()->getHandles());
        }

We can see this: $this->getUpdate()->getCacheId(), that again is based on current layout handles... Unfortunately, this cache id now exists in cache: it was generated by the ajax request with the same handles, and when that cache was saved, didn't include the correct page structure, since it did not load the cms page to load it's custom layout updates! So an invalid page structure is loaded from cache, without our added block, and it's used to render the elements.

I've put this example with a cms block included via custom layout update, but it's a potential issue for the rest of layout updates coming from db, that may get overriden eventually by this ajax request. That's why, by adding a 'dummy' handle for that ajax request, handle that is not intended to be used by anyone else, cache id gets unique for all ajax requests of that type, so it never conflicts with regular requests, that will be forced (if needed) to generate its own data.

Last paragraph contains the answer to question 2. It's not a constant because it is not intended to be used anywhere else (excluding Test classes) nor referenced nowhere, it's a private property of that class for private use.

I hope this explanation helps ;)

adragus-inviqa · 2017-05-10T06:57:09Z

Many thanks for the write-up. I get it now.

One small suggestion: if you think that dummy handle name is unique enough that it won't conflict with modules etc., that's fine. But if you think there's a chance of future conflict, rename it to mage_pagecache_additional_handle.

In its current form, it sounds useful enough to be actually used for other purposes.

adrian-martinez-interactiv4 · 2017-05-10T10:30:58Z

@adragus-inviqa I like your suggestion, I have updated the pull request: 1bb25ed

miguelbalparda · 2017-05-13T09:30:21Z

@adrian-martinez-interactiv4 thanks a lot for the submission! I'd like to merge this, I'll try to get somee xtra feedback from the issues you mentioned and will get back to you in case I have more questions.

adrian-martinez-interactiv4 · 2017-05-17T11:46:08Z

Hi @miguelbalparda, it seems that related issues #8554 and #9050 have been resolved in develop branch in another PR, as explained in #8554 (comment), so that issues are not related anymore with this one. This issue explained isn't solved yet in develop branch, I have updated this PR to include support for layout cache keys and now it's already fixed. If you need more feedback about this one don't hesitate to ask for it ;)

bejoycorra · 2017-06-27T07:03:40Z

Where can we get magento patch for this solution ? what is the patch id or URL?

ishakhsuvarov · 2017-07-27T10:52:54Z

lib/internal/Magento/Framework/View/Layout/ProcessorInterface.php

+     * @param array|string $cacheKey
+     * @return ProcessorInterface
+     */
+    public function addCacheKey($cacheKey);


To maximize backwards compatibility with the existing customizations and extensions we can not introduce a new public method in the interface. More information is available in the Backwards Compatible Development Guide

ishakhsuvarov · 2017-07-27T10:55:14Z

lib/internal/Magento/Framework/View/Model/Layout/Merge.php

+        }
+        $this->cacheKeys = array_merge($this->cacheKeys, $cacheKeys);
+
+        return $this;


Allowing method chaining in the classes which are not designed for it may introduce Temporal Coupling in the client code, therefore it is not encouraged.

ishakhsuvarov · 2017-07-27T10:57:10Z

lib/internal/Magento/Framework/View/Model/Layout/Merge.php

+     *
+     * @var array
+     */
+    protected $cacheKeys = [];


Currently Magento Technical Guidelines do not encourage designing classes for inheritance. Would be great to make this property private.

okorshenko · 2017-08-25T22:15:14Z

HI @adrian-martinez-interactiv4
Could you please look at @ishakhsuvarov comments? Thank you

ishakhsuvarov · 2017-08-31T15:34:08Z

Hi @adrian-martinez-interactiv4
Would you still like to proceed with this PR or should we close it? Do you need any assistance from our side?
Thank you.

miguelbalparda · 2017-09-19T14:41:06Z

According to contributor guide, tickets without response for two weeks should be closed.
If this issue still reproducible please feel free to create the new one: format new issue according to the Issue reporting guidelines: with steps to reproduce, actual result and expected result and specify Magento version.

vrann · 2017-09-21T14:41:27Z

Related to support ticket #MDVA-5525

…ento#8554 magento#9050 magento#9560 Create cache key object to be injected separately with its own interface

ishakhsuvarov · 2017-09-21T16:39:18Z

lib/internal/Magento/Framework/View/Layout/CacheKeyInterface.php

+/**
+ * Interface CacheKeyInterface
+ */
+interface CacheKeyInterface


Please consider more specific naming.

Ok, I'll try to find a more specific name.

Before refactoring, I'd like to ask, LayoutCacheKeyInterface sounds like an appropiate name?

ishakhsuvarov · 2017-09-21T16:39:45Z

lib/internal/Magento/Framework/View/Layout/CacheKeyInterface.php

+     * Add cache key for generating different cache id for same handles
+     *
+     * @param array|string $cacheKey
+     */


return type is missing

This method returns nothing, should it be noted as @return void?

Yes, docBlock should be clearly informing on that

ishakhsuvarov · 2017-09-21T16:41:55Z

lib/internal/Magento/Framework/View/Layout/CacheKeyInterface.php

+    /**
+     * Add cache key for generating different cache id for same handles
+     *
+     * @param array|string $cacheKey


This method may accept array, to be consistent with getCacheKeys

This method already accepts array as parameters as specified in @param notation, do you mean methods names are not consistent between them? (addCacheKey vs. getCacheKeys)

Or maybe addCacheKey should be called addCacheKeys if it accepts array?

ishakhsuvarov · 2017-09-21T16:42:19Z

lib/internal/Magento/Framework/View/Layout/CacheKeyInterface.php

@@ -0,0 +1,26 @@
+<?php


Please use PHP 7.0 type hinting syntax when possible

This one I just don't know what is it about, can you be a little more specific please? Thanks!

…ento#8554 magento#9050 magento#9560 Renamed interface, LayoutCacheKeyInterface made optional in constructor, injected via di.xml, some other little fixes

…ento#8554 magento#9050 magento#9560 Adapted PageCache and Framework tests

adrian-martinez-interactiv4 changed the title ~~Fix PageCache: async rendering of blocks can corrupt layout cache #8554~~ Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9550 May 9, 2017

adrian-martinez-interactiv4 changed the title ~~Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9550~~ Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 May 9, 2017

maksek added this to the May 2017 milestone May 9, 2017

maksek added the develop label May 9, 2017

miguelbalparda self-assigned this May 13, 2017

This was referenced May 13, 2017

PageCache: async rendering of blocks can corrupt layout cache #8554

Closed

Randomly getting an empty <body> that gets cached #9050

Closed

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

okorshenko modified the milestones: June 2017, July 2017 Jul 2, 2017

ishakhsuvarov reviewed Jul 27, 2017

View reviewed changes

ishakhsuvarov added the Progress: needs update label Jul 28, 2017

okorshenko modified the milestones: July 2017, August 2017 Aug 1, 2017

okorshenko assigned ishakhsuvarov Aug 25, 2017

magento-engcom-team modified the milestones: August 2017, September 2017 Sep 18, 2017

miguelbalparda closed this Sep 19, 2017

vrann added escalation labels Sep 21, 2017

miguelbalparda reopened this Sep 21, 2017

ishakhsuvarov reviewed Sep 21, 2017

View reviewed changes

adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Sep 22, 2017

Fix PageCache: async rendering of blocks can corrupt layout cache mag…

3cc51dc

…ento#8554 magento#9050 magento#9560 Adapted PageCache and Framework tests

adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Sep 22, 2017

Fix PageCache: async rendering of blocks can corrupt layout cache mag…

2ae9c26

…ento#8554 magento#9050 magento#9560 Adapted PageCache and Framework tests

adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Sep 22, 2017

Fix PageCache: async rendering of blocks can corrupt layout cache mag…

516b529

…ento#8554 magento#9050 magento#9560 Adapted PageCache and Framework tests

adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Sep 22, 2017

Fix PageCache: async rendering of blocks can corrupt layout cache mag…

efa3edb

…ento#8554 magento#9050 magento#9560 Adapted PageCache and Framework tests

adrian-martinez-interactiv4 added a commit to adrian-martinez-interactiv4/magento2 that referenced this pull request Sep 22, 2017

Fix PageCache: async rendering of blocks can corrupt layout cache mag…

3f60168

…ento#8554 magento#9050 magento#9560 Adapted PageCache and Framework tests

vrann self-assigned this Sep 22, 2017

osrecio mentioned this pull request Sep 27, 2017

Prevent invoice cancelation multiple times #11073

Merged

4 tasks

adrian-martinez-interactiv4 closed this Oct 1, 2017

adrian-martinez-interactiv4 force-pushed the develop branch from 3f60168 to 205a408 Compare October 1, 2017 23:22

adrian-martinez-interactiv4 mentioned this pull request Oct 1, 2017

Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 #11174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 #9560

Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 #9560

adrian-martinez-interactiv4 commented May 9, 2017 •

edited by vrann

Loading

adragus-inviqa commented May 9, 2017

adrian-martinez-interactiv4 commented May 10, 2017 •

edited

Loading

adragus-inviqa commented May 10, 2017

adrian-martinez-interactiv4 commented May 10, 2017

miguelbalparda commented May 13, 2017

adrian-martinez-interactiv4 commented May 17, 2017 •

edited

Loading

bejoycorra commented Jun 27, 2017

ishakhsuvarov Jul 27, 2017

ishakhsuvarov Jul 27, 2017

ishakhsuvarov Jul 27, 2017

okorshenko commented Aug 25, 2017

ishakhsuvarov commented Aug 31, 2017

miguelbalparda commented Sep 19, 2017

vrann commented Sep 21, 2017

ishakhsuvarov Sep 21, 2017

adrian-martinez-interactiv4 Sep 21, 2017

adrian-martinez-interactiv4 Sep 21, 2017

ishakhsuvarov Sep 21, 2017

adrian-martinez-interactiv4 Sep 21, 2017

ishakhsuvarov Sep 21, 2017

ishakhsuvarov Sep 21, 2017

adrian-martinez-interactiv4 Sep 21, 2017

ishakhsuvarov Sep 21, 2017

adrian-martinez-interactiv4 Sep 21, 2017

Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 #9560

Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 #9560

Conversation

adrian-martinez-interactiv4 commented May 9, 2017 • edited by vrann Loading

adragus-inviqa commented May 9, 2017

adrian-martinez-interactiv4 commented May 10, 2017 • edited Loading

adragus-inviqa commented May 10, 2017

adrian-martinez-interactiv4 commented May 10, 2017

miguelbalparda commented May 13, 2017

adrian-martinez-interactiv4 commented May 17, 2017 • edited Loading

bejoycorra commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okorshenko commented Aug 25, 2017

ishakhsuvarov commented Aug 31, 2017

miguelbalparda commented Sep 19, 2017

vrann commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-martinez-interactiv4 commented May 9, 2017 •

edited by vrann

Loading

adrian-martinez-interactiv4 commented May 10, 2017 •

edited

Loading

adrian-martinez-interactiv4 commented May 17, 2017 •

edited

Loading