Problem/Motivation
Most webservers have a maximum size for headers.
Proposed resolution
We could implement an (optional) hashing strategy that would significantly reduce the size of the headers that are sent out.
Acquia purge already includes a similar solution.
| Comment | File | Size | Author |
|---|
Issue fork purge-2952277
Show commands
Start 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:
Comments
Comment #2
bucefal91 commentedhello!
Just recently I leveraged purge module for Varnish caching and we do hit the issue of too long headers (apache + mod_fcgi). The limit is 8kb and I think one has to re-compile apache in order to bump it (https://maxchadwick.xyz/blog/http-response-header-size-limit-with-mod-pr... , https://github.com/webdevops/TYPO3-metaseo/issues/233)
I read through #2844852: Minify the cache tags sent in the header and I understand the concerns of nielsvm. I still think running a minor risk of hash collision is better than running no Varnish on anonymous HTML :)
I would propose to introduce some kind of salt into the hash to cover the issues of collision. Considering https://www.drupal.org/project/acquia_purge/issues/2844852#comment-12513413 .. basically for the website developers is OK to have some collisions, it is very undesirable to have such a collision where one tag is updated very frequently and another is part of too many pages. Then it would invalidate too much of cache too frequently.
If developer encounters such case in his setup, he could just change the salt to kind of re-distribute the hashes in a new fashion. With certain (rather high) degree of luck, a new salt will change collisions and the "frequent" and the "popular" cache tags will no longer collide. It does require some thinking and analysis from the developer, but that's at least some kind of a solution. The salt could be kept in Drupal State API and we could expose some "reset salt" button on the admin UI.
Also, is there any particular reason md5() was used in https://www.drupal.org/project/acquia_purge/issues/2844852 ? I am by far not an expert in cryptography and hashing, but nowadays md5 used even in non-sensitive context will trigger warnings and questions upon a security audit. I checked, D8 core password hashing routine uses
hash('sha512')On a different note, there is also an alternative to pure "blind" hashing. Hitting 8kb limit is not so easy, after all, for a single page. Normally it should be some "family" of cache tags that "explode" the length of the header. In the case of https://www.drupal.org/project/acquia_purge/issues/2844852 it was taxonomy terms. In my case there are few such families:
paragraph:*,node:*. If our real estate (header size) is limited, we should optimize its consumption. We could abbreviate (compress) the most common (at least in Drupal core) tags to 1 letter. Things like (using regexp syntax to make it shorter):node:(\d+) => n:$1config:(.+) => c:$1block => btaxonomy_term:(\d+) => t:$1paragraph:(\d+) => p:$1Such rather primitive string substitutions can significantly help in shrinking length of the header. Also, they do not obfuscate matters as badly as md5 (that's to consider #2890018: Introduce a debug mode for tags not to be minified in the HTTP headers). They also expose a risk of collisions, but I think the risk of undesired collisions as I explained them above is smaller with the substring substitution. The patch I attach takes this approach - I introduce a minification service (so both Cache-Tag header and Purger plugins get to use the same logic on minification without code duplication). I also introduce a hook to collect the substring replacement in cache tags - through this hook one could tailor the replacements to his own specific case (in case minifying core tags is not enough and his website has lots of custom cache-tags). The patch works quite well on my localhost. I am going to run a few more tests and will deploy it onto the server where the website I am working on is hosted.
And the last thought... If your page has more than 1k cache tags (8 bytes per cache tag) than there is nearly no way to solve it by any kind of manipulation/minimifaction. I guess fundamentally proper way is to have Drupal "orchestrate" cache tag => page URL relation in some kind of key-value storage. This would significantly increase amount of HTTP Ban requests and it would be wise to research if there's any limit on the BAN requests that most of proxy caches pose (Varnish, CloudFlare, Nginx, etc)
Comment #3
bucefal91 commentedComment #4
bucefal91 commentedMore thorough testing reveals a few more sweat spots for minifying bytes :)
Comment #5
quentin massez commentedHello,
I had a problem of too large header due to the size of cache-tags.
This patch solved my problem. Thanks !
I also noticed something on cache tags that seems to me unoptimized : I had both user_list and user:uid cache tags.
But it's not really related to this issue and I should probably open another one.
Comment #6
gaëlg#3001276: Remove individual entity cache tags when an entity list cache tag is here (no need for node:42 if node_list is here)
Comment #7
gaëlgFor now the minification from patch #4 seems to be enough for our use case (not that much tags). I change this to "reviewed" because of Quentin's feedback. Thank you @bucefal91!
If it happened not to be enough in the future, there could be another optimization: remove every "node:XX" tag when there's a "node_list" tag (and the same for terms, etc.). It seems like this cannot be done easily in core, as I just figured out in #3001276: Remove individual entity cache tags when an entity list cache tag is here (no need for node:42 if node_list is here).
Comment #8
bucefal91 commentedI am glad my patch was useful to others too. From my side, let me also mention that we've been running my patch #4 in production (D8 + Purge + Varnish setup) for half a year now. No issues came up.
Comment #9
miroslavbanov commented1. Why is there the #2 patch file committed as part of the #4 patch?
2. Should we add a Null CacheTagMinificator implementation, at least for debugging purposes, in case someone suspects issues with the minification?
3. Also please note that acquia_purge and cloudflare are using hashing. I don't think hashing should be dismissed easily, I think it is very effective, and it might be implemented in a follow-up issue and become a popular alternative service implementation.
Comment #10
gaëlg@bucefal As our problematic website continued to grow, we hit the max header size again, even with the minifier. So that I implemented what I told in #7, as an addition to #4.
@MiroslavBanov
1. It's presumably a mistake.
2. A Null CacheTagMinificator implementation might be useful, as a follow-up issue maybe?
3. Yes, a follow-up issue for hashing might be useful. Personally I like this minification method because in case of bugs, tags are still somewhat "human-readable".
Comment #11
gaëlgComment #12
miroslavbanov commentedAgreed, #9.2 and #9.3 can be implemented in a follow-up issue.
Nice that we are making this effort to optimize. I don't think that the currently proposed solution matches my needs very effectively, but that can be fixed in follow-ups. Cheers!
Comment #13
wim leersI did something similar a long time ago at #2491561: Port to Drupal 8.
Comment #14
almunningsI noticed that my tags weren't getting minified after patch #10 when sent back to varnish for a BAN command.
Should the [invalidation:expression] token perhaps call in a minifyCacheTag() ?
Comment #15
rp7 commentedI can confirm #14. After applying this patch, Varnish purges no longer work because the non-minified/simplified cache tags are sent in the BAN request.
The issue arises because the tokens [invalidations:separated_pipe], [invalidations:separated_comma] & [invalidations:separated_tab] don't use the minify/simplify services yet. Added the follow lines (having issues creating an interdiff) in purge_tokens_tokens():
Updated patch attached.
Comment #16
gaëlg@rp7 Nice catch! Here's an update of patch #15 so that it also works with the [invalidation:expression] token.
Comment #17
ndobromirov commentedThis should handle
menu_link_content_listas well.Very often all pages that have headers and footers will have this and at least 10s of menu-links in there.
Maybe consider the shortening of:
menu_link_content:39to either of:m_l_c:39mlc:39ml:39I think it's this module's responsibility to support all entities that are in core and can go into the tens of items on a page.
Please consider this core issue as well #2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing.
Comment #18
ndobromirov commentedOverall thanks for this initiative.
With a manual test of all of the above ideas, I've managed to shrink a particular page header from 1760 bytes to 680
Media items should be minified as well.
media:1->md:1,ma:1orm:1.Comment #19
ndobromirov commentedHere are the proposed changes for the menu_link_content entities and their list tag.
Extended new interfaces doc-blocks with high level descriptions and examples.
Small tweaks here and there.
Adding needs tests tag due to the fact that the 2 new services are not unit-tested.
Comment #20
ndobromirov commentedComment #21
ndobromirov commentedHere is another version that support media minification.
As medias are way more used compared to the menus.
I am switching the replacements a bit and now
media:gets them:token prefix.Comment #22
rp7 commentedPatch in #15 for fixing the invalidations tokens applies the simplifier to the tokens. This transforms
[ 'node:123', 'node_list' ]to[ 'node_list' ], which is obviously incorrect. Responses can very well be cached with only thenode:123cache tag, but not thenode_listcache tag.Removed this line, so now only the minificator is used for invalidating.
Comment #23
jonhattanAdded this minifications for core:
Also, I think
paragraph:should be out since it is not core, but anyway changed toparagraphto also matchparagraph_wrapper. If we're going to add common contrib modules, page_manager deserves a shorten (page_manager_route_name:page_manager.page_v_home).Also added a simplification for
media_list.Comment #24
jonhattanThere was a typo in my last patch
Comment #25
jonhattanAnother typo (responsive_images -> responsive_image)
Comment #26
miroslavbanov commentedThe solution is obviously very needed and a step up for a lot of sites. But I see a problem here that there is no way to disable any of this, except in code.
Comment #27
vtcore commentedRerolled #25 for the latest 8.x-3.x-dev (commit 19adf78)
Comment #28
mpp commentedAdded missing newline in purge.services.yml and rerolled against latest dev.
Comment #29
willeaton commentedThis might need a rerole, it returns patch errors now. Just commenting for now.
Comment #30
seanbRerolled.
Comment #31
seanbSorry, #30 contains a mistake. This is a correct reroll.
Comment #32
adamfranco commentedI've been using #31 for several days after running into huge headers caused by the Group module's many cache tags (its many-to-many relationships add tags for all three of the group, the group-content mapping-entity, and the content entity). This patch and the custom hook implementations below have reduced my cache-tag headers from the 8KB-17KB range down to 300B-3KB, a drastic improvement. I think this patch strikes the right balance of doing just enough to be helpful out-of-the-box while still being extensible and customizable to fit special needs.
The only potential improvement I can think of would be to make minification a configurable option -- I'd always want minification myself and I think it is a good default, but maybe others have a use case for the raw cache-tags.
Comment #33
lisa.rae commentedMarking Needs Work, patch is failing testing.
Comment #34
idebr commented#33 The tests have been failing since Aug 2017, see https://www.drupal.org/pift-ci-job/1595419. This issue still needs tests though, so the issue status change is valid regardless.
Comment #35
lendudeDev tests should be fixed now by #3035053: Dev builds are broken., so requeued #31 to see if that doesn't break anything existing
Comment #36
kiseleva.t commentedI've applied the patch, but in some cases, the Cache-Tags header is still too big.
Changed that similar to the Acquia Purge module to use hashes.
Comment #37
miroslavbanov commentedAbout patch #36, I actually contributed a bit regarding the hash function, but it is less optimal as it's a base32, and that means more collisions. Here is the base64 version of it. This has 16777216 unique hashes for a 4 character base64 hash, rather than the 1048576 for a 4 character base32 hash.
Of course, in terms of a solution it's still not perfect - there can be collisions, there is still no way to disable it, and no way to switch to a different minification approach.
Comment #38
kiseleva.t commentedFixed small syntax error in patch #37 (appears in #36).
Comment #39
hugronaphor commentedI confirm that #38 solved the issue for me.
Comment #40
adamfranco commentedEspecially as this patch drifts away from human-readable key-minification into more opaque hashes, I'm feeling like maybe it is time to make the minifier a plugin that can be configured or swapped. @MiroslavBanov's idea of a null-minifier would allow disabling minification, a key-only minifier (patch #31) could be used by those for whom it is sufficient and want to avoid collisions, and a hashing minifier (patch #38) could be optimized for absolute minimum size at the expense of collisions causing potentially unnecessary purges. Making minification optional would hopefully also allow for easier real-world testing of the different strategies since site administrators could change the setting live (with a cache rebuild) rather than needing to patch code and redeploy.
Comment #41
fagoRunning into this issue as well.
I think adding pluggability is not necessarsy at this mount when all we would like is an option to disable this during development. So let's simply add a config option or even just a setting for it?
Comment #42
sutharsan commentedAccording to my calculation this hash length results in a high risk of collision.
Using the calculation at https://preshing.com/20110504/hash-collision-probabilities/
I come with the following:
4 char hash: 1638 hashes: chance of unique 1.097E-9
6 char hash: 1170 hashes: chance of unique 0.960
7 char hash: 1023 hashes: chance of unique 0.998
8 char hash: 909 hashes: chance of unique 0.99990
Based on this, I propose a hash length of 8 chars.
Disclaimer: I'm a statistics noob.
Comment #43
miroslavbanov commentedMaking the hash longer makes the minification less effective. Could be better to have the option to configure the length.
Also note that a collision is usually not the end of the world. Some cached items would just be invalidated when they shouldn't.
Comment #44
aspilicious commentedThis saved my life :)
Comment #45
mpp commentedFixed merge conflict.
Comment #47
mpp commentedI created an issue fork to facilitate further development on this issue. A patch is available on https://git.drupalcode.org/project/purge/-/merge_requests/5.diff
It would be useful to continue the work there.
Comment #48
astoker88 commentedRe-rolled the patch to work against latest branch - PHP 7.4 Drupal 9.2.6
Comment #49
astoker88 commented#48 is broken fixed up here. @mpp didn't see yours until after id posted, so #49 is just your diff!
Comment #50
bramdriesenInteresting, I'll also test this in a couple of days :)
Comment #51
astoker88 commentedLooking through the patch here, we are applying the cache tags minification to other types of invalidators.
We use a combination of "tag" and "path", the tag purger works, however just noticed that the path invalidator is always returning a blank value, causing a lack of invalidation.
Suggest we add a check so that the token replacement minification happens only when the type is tag, not path or any other scenario.
Comment #52
gaëlgWe still had problems on very long views pages (maps) when using latest patches. It's because the simplification logic has been removed
since patch #36. So I made a little module which adds it back: https://www.drupal.org/project/cache_tags_simplify
Comment #53
jmaties commentedI think you are right adamfranco in #40
I have mixed both patches (#31 and last) and make it configurable.
Comment #54
uros.ceh commentedRe-rolling the patch #38 for compatibility with 3.4 module release.
Comment #56
renrhafPatch works well, no more 502 errors because headers are too big for Nginx.
Thank you.
Comment #57
rp7 commentedA few changes to patch in #53:
Comment #58
masipila commentedI'm testing (and reading) the patch #57.
Could somebody please elaborate once more what's the conceptual logic behind this?
Isn't it so that:
So are we throwing away the node:xxx tags if and only if also the node_list tag is also present? In other words, the individual node pages would still have the node:xxx cache tags?
If my thinking-out-loud is correct here, can we please elaborate the documentation block of purge_purge_cache_tags_simplify_dictionary()?
Cheers,
Markus
Comment #59
afinnarn commentedI'm not sure how helpful this comment is, but I tried the patch in #54 and got the following error.
I had to switch to other work and didn't have time to look into the error, but I will update the thread if I do. Posting since someone else might encounter the error or know how to fix in a patch.
Comment #60
afinnarn commentedOkay, the relevant patch in the Acquia Purge module is here and it says to re-roll into this patch: https://www.drupal.org/project/acquia_purge/issues/3157178
I will look into this now in my local dev environment.
Comment #61
nickdjmRerolled #57 for latest version.
Comment #62
bceyssensI'm not a fan of the hashing solution as you have no idea what the content is. The dictionary looks like a lot of work to maintain.
As an alternative method I added compression strategy, keeping only the first letter of each word. It is fully automated and you could still retrieve the original cache tag.
As you should still have the option to disable minification logic completely, I changed the configuration form to a select.
Example minitication ratios (number of characters in the cache tags header):
Comment #63
bceyssensReplacing incorrect patch
Comment #64
swatigarg06 commentedI tested patch #63 and it seems to be failing for 8.x-3.4.
Comment #65
mattbloomfield commentedI rerolled an older patch for the new version of the module.
Comment #66
mattbloomfield commentedPlease disregard the last one, here's a new patch
Comment #67
andypostComment #68
dasginganinjaI just went through the past issues and tried to get an understanding of what's going on in this thread. Hopefully this summary helps guide this issue towards an actual patch.
I was starting with patch #49.
There is a MR for this thread that is based on #49 available here: Merge Request !5
Notes for #49 patch / #66
- Patch by default uses MD5 with first 4 characters in #49 and what is expected with this patch for those who has applied it
- Patch 66 seems like it's #49 version rerolled
- Patch #51 can be added to #49 to limit it to just being tags. This is an issue with path invalidation cases and should be applied.
- This does not have any configuration / schema
New functionality / schema changes in #61/63
- Seems like there is a lot of configurability in these -- good work!
- 61 vs 63 was the latest comparison for the _NEW_ work for alternate methods of shortening tags
- Both contain dictionary and MD5 approaches
- 63 has a new "compress" function as well as all the previous work (except #50?)
- 63 changes from simplification_logic to minification_logic configuration key. This means that anybody on simplification_logic key will need to migrate to that. Perhaps we fall back to the previous simplification_logic key?
- 63 will allow the original tag through if there is no config or it is set to null
- 61 will default to md5 if the simplification_logic config is not present
Missing functionality overlooked in #51 as this logic applies to path type expressions as well. This should be incorporated into all of these patches.
I think it would be great if #61 / #63 could be combined into a similar request, using the simplification_logic key. The code in #63 is a lot easier to read FWIW.
I hope this helps move things forward.
Comment #69
druplr commentedHere is @mattbloomfield's patch #66 (which as @dasginganinja indicated, is #49 rerolled) with whitespaces corrected. This does not include the new functionality / schema changes in #61/63.
Comment #70
mattbloomfield commentedThanks druplr. I used your patch to reroll #61. I saw #63 was failing, so I'm not sure what we might want to do with that.
Comment #71
michel.g commentedAdd a check on TagInvalidation because this code was also replacing tokens on other validators (e.g. URL)
Comment #72
j_bekaert commentedCombination of #71 with the added functionality of #63.
Comment #74
prudloff commentedI updated the MR with the latest patch from comment 72.
We still need to add tests.
I also feel like this is getting hard to review.
It might be best to split this into several smaller issues (one for the base system and one for each minification strategy for example).
Comment #75
jelle_sIt seems like #52 got overlooked. Somewhere in this discussion the simplification logic was removed from the patch, while some hooks dealing with simplification are still in there. I would suggest adding the simplification of the dictionary back in, minification by the dictionary alone seems to not be enough in some cases.
Comment #76
jelle_sI re-added the simplify logic and added a setting for it.
Side note: I believe the form for these settings and the route to that form should be moved to the purge_ui module?
Comment #77
jelle_sPatch didn't apply to 3.6 for me, this one should
Comment #78
tuwebo commentedHello,
I've just run into this issue. Given that #3395776: Make POST requests render cacheable I was wondering what is going to happen when using the "Mignification logic: Compress", since it looks like the core RenderCache::get is hardcoding "CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD" and the mignification logic is converting it as cachemissifuncacheablehttpmethod:f
Should I use the dictionary logic instead or is there any other configuration that I am missing?
Comment #80
recrit commentedUploading a static patch for the latest MR updates that supports simplifying
{ENTITY TYPE}_list:{ENTITY BUNDLE}. Example: "node_list:article".Comment #81
recrit commentedAfter doing some more testing with "Simplify cache tags" enabled, there is a fundamental issue with this feature.
The cache tag header is used by a Varnish to tag the cached response that it receives from Drupal. When cache tag simplification used, the "{ENTITY_TYPE}:{ENTITY_ID}" tag (e.g. "node:1234") is removed from the header value when the node is saved.
For example:
- Drupal cache tags for the headers: "node:1234", "node_list" . (EDIT 1) The list tag is being added by a block / paragraph on the page that displays a list of related nodes.
- Simplified tags: "node_list"
- RESULT = BUG: Varnish does not tag the node page with it's own tag "node:1234". So if the Drupal cache tag "node:1234" is invalidated by itself, then the Varnish page is not invalidated. You have to invalidate it with "node_list" which is wrong since it clears all pages.
EDIT 1: Clarified how the "node_list" Drupal cache tag was being added to the page.
Comment #82
prudloff commentedI don't think it's a bug, node_list is invalidated when any node is updated, so node:1234 is redundant here.
When a node is updated, Drupal invalidates both the node:xxx and node_list tags.
Comment #83
recrit commented@prudloff Not a hard bug, but maybe a warning / caveat for others. You can no longer run ` drush p:invalidate tag node:1`, it will not work. Anyone using "Simplify cache tags" should evaluate their pages to confirm the expected cache tags are present.
For my use case, I was able to use `hook_purge_cache_tags_simplify_dictionary_alter()` to ensure that the current page's entity context had tags for the node, group content / relation, and group. So that clearing any of those specific tags, clears those pages.
Comment #84
geek-merlinJudging from the IS, this is a dup of #2844620: Automatically split cache debug headers into multiple lines when they exceed 8k, which fixes the problem in all cases, and is already rtbc.
Comment #86
driskell commented@geek-merlin I think this is related and a good solution but not a duplicate as that issue only impacts the debug headers but this issue was about the Cache-Tags header the contrib module appends in its TagsHeader plugins. The same solution could be used potentially.
Comment #87
seanbMaybe this is unrelated, but we recently had an issue with large headers in our tests. We got nginx throwing the infamous
upstream sent too big header while reading response header.For us, this was triggered by the
_drupal_error_header()function, adding aX-Drupal-Assertionfor each error or notice in the tests. After the Drupal 11 update, we had a lot of deprecated code that is now still working, but needs to be fixed before we update to Drupal 12. Having a lot of these notices caused nginx to throw the error.Obviously, we need to fix the deprecations. For now, we added a workaround in our
settings.local.phpfor the tests.Just posting this here since it was very difficult to debug this weird nginx error for our tests. Hope this helps someone.
Comment #88
jan kellermann commented@geek-merlin
This would be the patch:
But Apache doesnt merge the headers to one header line. If you want to split up the header, you have to modify the ban-vcl also:
Not sure if this is the best way or whether we should keep trying to reduce the size of the cache tags.
Comment #90
duaelfrSome plugins, like the purger from the Bunny CDN module, might use the
getExpressionmethod onTagInvalidationobjects instead of casting them to strings. I updated the MR to have the same processing on this method as the __toString() method.Comment #91
duaelfrNow tags are minified (or not, depending on configuration) directly in
TagInvalidation::getExpression(). Calls topurge.cache_tag_minificatorare not needed anymore inhook_tokens_tokens.Comment #92
duaelfrPatch provided for composer.
Comment #93
falco010Hi @duaelfr,
We were using the MR diff of this issue as a patch in our project.
After your changes in the MR, the functionality of this issue seemed to be broken (Not sure which part/commit from you). It would not be properly invalidated anymore. As extra info: we are using the 'Hash (using MD5 algorithm)' Minification logic.
After switching back to the patch in #80, the invalidation and functionality of this issue continued to work properly again.