Split X-Magento-Tags HTTP header into multiple headers #12831

sambolek · 2017-12-20T14:04:54Z

On Magento sites with large catalogs, it's easy to hit HTTP header length limit on Varnish or HTTP server, for categories that have large amount of products and are set as anchor. This is because all of the product tags are sent in initial backend_fetch request.

This is usually addressed by implementing a workaround from the documentation http://devdocs.magento.com/guides/v2.2/config-guide/varnish/tshoot-varnish-503.html . Thing is, that workaround doesn't solve the issue, as you end up increasing the limit constantly as categories grow larger.

Furthermore, you might hit another limit on your Apache/nginx HTTP server, as both have 8kB max header value by default.

In M2.2 2529ec4 , cache tags have been refactored to be a bit shorter (e.g. cat_p_xx instead of catalog_product_xx), which is another workaround to address the same issue.

According to RFC2616 https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma.

Description

This pull request proposes splitting the X-Magento-Tags header into multiple X-Magento-Tags headers that would stay roughly under 8kB limit each.

Fixed Issues (if relevant)

Manual testing scenarios

Set category as "anchor" and assign about 2500 products to it (it's important that X-Magento-Tags header goes over 8kB limit)
Set up Magento store to use Varnish, with default Magento-generated VCL and no other parameter configured (ignore workaround/recommendations from this page http://devdocs.magento.com/guides/v2.2/config-guide/varnish/tshoot-varnish-503.html or just assign 2500 more products to the same category)
Attempt to open a category on frontend
Category returns 503 error from Varnish
If you have varnishlog active at the time this occurs, observe the error FetchError http read error: overflow on bereq 2 fetch request

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

sambolek · 2017-12-20T14:07:17Z

I'm open to recommendations for this one - especially the part adding header to StaticMap. It appears a bit difficult to add another header mapping to Zend\Http\HeaderLoader, as this has to be done before first header is set on response.

Header splitting might also be improved, but curious to see if this is the right approach to solve this issue.

…void issues with max header length

sambolek · 2018-01-12T12:28:27Z

Any news about this one?

sambolek · 2018-02-25T01:57:01Z

@magento-team @magento-engcom-team

Anyone caring enough to discuss this? Don't think we should be sending MBs inside a single X-Magento-Tags header, but keep to hear what you think.

ihor-sviziev · 2018-03-24T20:52:52Z

@kokoc @paliarush, you're responsible for page cache module. Could you review this PR?

mropanen · 2018-06-04T08:25:30Z

https://github.com/magento/magento2/pull/12831/files/05ff2db5923c61d3a6a0efa374ce2ded3af8cc93#diff-8824a34ec801e0f52b1299f54ac9104cR66

I think you mean to implode $newTags, not $tags there :)

ihor-sviziev · 2018-06-05T07:41:35Z

@sidolov could you review these changes?

ishakhsuvarov · 2018-06-08T14:46:10Z

Hi @sambolek
Sorry for a late review. I've got a few questions regarding the implementation:

What is the specific reason to split only one header? If there is a real-world example and described in RFC this may be implemented for any headers or for some configurable.
If this problem is applicable exclusively to specific catalog setups why not reconfigure varnish?

sambolek · 2018-06-08T22:19:30Z

Hi @ishakhsuvarov ,
the issue we actually had couldn't be easily traced, but turned out to be related to X-Magento-Tags header being too large for some part of the system. It might be due to some other limit that isn't documented in Magento docs, but we (nor the hosting company) have figured it out.

As per my initial message, if a category has about 2500 products, a single header X-Magento-Tags gets too large, causing issues with Varnish (having to adjust parameters all around), web server (Apache and nginx have 8kB header limit), and LoadBalancers (HAProxy and nginx have 8kB limit as well). Even if there is a limit to adjust, the solution isn't to keep adjusting the limit as you add more products to the system and assign them to categories.

Many merchants could easily have categories with large amount of products, or anchor categories with plenty subcategories - since Magento is targeting mid/large merchants, catalog should work regardless of these factors.

Since RFC allows for multiple headers with the same name, the logical way forward would be do build the application (Magento) in a way that headers are split properly, without a need to adjust limits on the entire system.

This pull request solves the problem at hand, without going too deep into architectural choices of Magento system/platform. Of course, it might be wise to explore splitting all cookies that can be comma-separated (as per RFC) on platform level, but, in the meantime implement this pull request, which shouldn't have any BC to help other merchants that have the same issue we did.

ishakhsuvarov · 2018-06-13T08:13:13Z

Hi @sambolek

This pull request solves the problem at hand, without going too deep into architectural choices of Magento system/platform. Of course, it might be wise to explore splitting all cookies that can be comma-separated (as per RFC) on platform level, but, in the meantime implement this pull request, which shouldn't have any BC to help other merchants that have the same issue we did.

This PR indeed, seems to solve the problem. However, it could be better to try and implement the desired state approach right away, as fixing only one header type just masks the problem.

I'll submit more thoughts on it via the code review to the Pull Request to continue the discussion.

ishakhsuvarov

To accept this solution to the core I would suggest making it more generic, as current state of it seems to be a perfect solution for a third-party module, but not a core component

ishakhsuvarov · 2018-06-13T08:23:44Z

app/code/Magento/PageCache/Model/HTTP/PhpEnvironment/ResponsePlugin.php

+     *
+     * @return Subject|mixed
+     */
+    public function aroundSetHeader(Subject $subject, \Closure $proceed, $name, $value, $replace = false)


It's not recommended to add new around plugins, as it has a greater performance penalty, specifically, in such a critical part of the system.

ishakhsuvarov · 2018-06-13T08:25:07Z

app/code/Magento/PageCache/Model/HTTP/PhpEnvironment/ResponsePlugin.php

+
+        $this->addHeaderToStaticMap();
+
+        if ($name == 'X-Magento-Tags') {


There are already plugins in the system which set X-Magento-Tags. Did you consider updating those instead of processing same header again?

ishakhsuvarov · 2018-06-13T08:26:37Z

app/code/Magento/PageCache/Model/HTTP/PhpEnvironment/ResponsePlugin.php

+    public function aroundSetHeader(Subject $subject, \Closure $proceed, $name, $value, $replace = false)
+    {
+        //if varnish isn't enabled, don't do anything
+        if (!$this->config->isEnabled() || $this->config->getType() != Config::VARNISH) {


While the issue described in the Pull Request only relates to Varnish, this header is also used by Builtin Implementation. Would it make sense to make it more generic?

ishakhsuvarov · 2018-06-13T08:27:46Z

app/code/Magento/PageCache/Model/HTTP/PhpEnvironment/ResponsePlugin.php

+                $newTags[] = $tag;
+            }
+
+            return $subject;


Different plugins, for example solving similar issue for other header types may be theoretically broken in this way. Does it make sense to tune the actual Response implementation instead?

ishakhsuvarov · 2018-06-13T08:29:38Z

app/code/Magento/PageCache/Model/HTTP/Header/XMagentoTags.php

+     * @throws Exception\InvalidArgumentException
+     * @return string
+     */
+    public function toStringMultipleHeaders(array $headers)


This method seems to be copied from the Zend's default implementation of \Zend\Http\Header\GenericMultiHeader::toStringMultipleHeaders which could make it more challenging to support in the future

ishakhsuvarov · 2018-06-13T08:30:26Z

app/code/Magento/PageCache/Model/HTTP/Header/XMagentoTags.php

+     */
+    public function getFieldName()
+    {
+        return 'X-Magento-Tags';


X-Magento-Tags literal occurs many times across this this PR, would it make sense to move it to a constant?

sidolov · 2018-08-05T10:57:58Z

Hi @sambolek , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

kurtdeserter · 2020-01-08T11:11:01Z

Are there any updates for this issue? This problem still exists in M2.3.3
All "fixes" found by now, are not resolving this issue.

ihor-sviziev · 2020-08-06T07:02:27Z

Would like to add some summary on this PR:

This solution is quite good, but it's not finalized in order to be accepted.
You could create new PR based on this PR and apply requested changes.

I have only one concern - such change might break some services like Fastly (for instance used in Magento Cloud), so probably it should be double checked additionally

hostep · 2022-06-01T12:44:17Z

It would still be nice if a solution for this could get implemented anywhere in the future for this problem.

We currently run against this huge X-Magento-Tags header on a Magento 2.4.3-p2 installation with the search results page when it returns > 5000 products or something like that in its resultset.

@ishakhsuvarov, @sidolov, are there plans to ever support this somehow? Not necessarily by splitting the header, but maybe by using some other (and fewer) cache tags in search results so it doesn't list every single product id from the resultset?

#26960 should also get re-opened in my opinion, since it's not fixed yet.

hostep · 2022-06-01T13:59:46Z

Sorry, ignore comment above, we had this issue because the pagination on search results pages didn't work, all products were always being displayed instead of only one page. So, once we solve this the pagination of search result pages, we won't run into this anymore.

Sorry for the noise 🙂

ihor-sviziev · 2022-06-01T14:04:30Z

@hostep we're having the same issue with showing all products on search. Were you able to find the reason for that issue?

hostep · 2022-06-02T07:54:19Z

@ihor-sviziev: in our case it was a custom module that loaded the product collection too soon before the pagination was getting applied (or something like that, still trying to figure out exactly is going on and how to fix it).

If you have this issue with all products being shown in the search result page, it's most likely not a core Magento bug, but some customisation done to your shop, either in the custom theme or in custom/3rd party modules. Try switching to a default theme and/or disabling modules and test again, until you find the culprit, that's how I found out about which module caused the issue in our case.

hostep · 2022-06-02T09:36:22Z

Update @ihor-sviziev, seems to have something to do with this change: #27263, when I add $this->addToolbarBlock($collection); back, it works again in our case.

Also possibly related issue: #34292

We use the catalog_block_product_list_collection event in our custom module to get the product collection, and after the PR change from above, the toolbar filtering is no longer applied before the event is dispatched, so we get the full collection and by looping over it we load the collection before the toolbar filters have been applied.

Sorry to the readers of this thread, this has nothing to do with it, I'll try to shut up now 😄

Split X-Magento-Headers into multiple headers when using Varnish to a…

05ff2db

…void issues with max header length

sambolek mentioned this pull request Mar 6, 2018

X-Magento-Tags header too large #6401

Closed

ihor-sviziev added the Release Line: 2.3 label Mar 24, 2018

udovicic approved these changes Apr 5, 2018

View reviewed changes

ishakhsuvarov added the Progress: needs update label Jun 8, 2018

ishakhsuvarov requested changes Jun 13, 2018

View reviewed changes

ishakhsuvarov removed the non-issue label Jun 13, 2018

sidolov closed this Aug 5, 2018

ihor-sviziev mentioned this pull request Nov 17, 2020

X-Magento-Tags Header too long - Apache / PHP error 500 on category pages #26960

Closed


		$this->addHeaderToStaticMap();

		if ($name == 'X-Magento-Tags') {

Split X-Magento-Tags HTTP header into multiple headers #12831

Split X-Magento-Tags HTTP header into multiple headers #12831

Uh oh!

Conversation

sambolek commented Dec 20, 2017 • edited by ihor-sviziev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

sambolek commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sambolek commented Jan 12, 2018

Uh oh!

sambolek commented Feb 25, 2018

Uh oh!

ihor-sviziev commented Mar 24, 2018

Uh oh!

mropanen commented Jun 4, 2018

Uh oh!

ihor-sviziev commented Jun 5, 2018

Uh oh!

ishakhsuvarov commented Jun 8, 2018

Uh oh!

sambolek commented Jun 8, 2018

Uh oh!

ishakhsuvarov commented Jun 13, 2018

Uh oh!

ishakhsuvarov left a comment

Choose a reason for hiding this comment

Uh oh!

ishakhsuvarov Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

ishakhsuvarov Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

ishakhsuvarov Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

ishakhsuvarov Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

ishakhsuvarov Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

ishakhsuvarov Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

sidolov commented Aug 5, 2018

Uh oh!

kurtdeserter commented Jan 8, 2020

Uh oh!

ihor-sviziev commented Aug 6, 2020

Uh oh!

hostep commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hostep commented Jun 1, 2022

Uh oh!

ihor-sviziev commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hostep commented Jun 2, 2022

Uh oh!

hostep commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sambolek commented Dec 20, 2017 •

edited by ihor-sviziev

Loading

sambolek commented Dec 20, 2017 •

edited

Loading

hostep commented Jun 1, 2022 •

edited

Loading

ihor-sviziev commented Jun 1, 2022 •

edited

Loading

hostep commented Jun 2, 2022 •

edited

Loading