[RFC] Added MaxHeaderValueLengthFormatter #424

Toflar · 2018-08-09T08:34:54Z

This is an idea on how we could try to solve the maximum length of the header values in the ResponseTagger. We already have TagHeaderFormatters so I thought I'll write one that takes an inner formatter and splits the tags of this one to make sure it does not exceed the max length. This would allow to use this formatter to limit the header value length of any other formatter (the built in CSV or your very own) very easily (a pretty cool concept I find 😄 ).

For this to work, however, I had to adjust the return value of the TagHeaderFormatter interface and I know that in theory this is a BC break. BUT: I wanted to discuss this here because it's really only a theoretical BC break. So what I did is I changed this:

    /**
     * Get the value for the HTTP tag header.
     *
     * This concatenates all tags and ensures correct encoding.
     *
     * @param array $tags
     *
     * @return string
     */
    public function getTagsHeaderValue(array $tags);

to this:

    /**
     * Get the value for the HTTP tag header.
     *
     * This concatenates all tags and ensures correct encoding.
     *
     * @param array $tags
     *
     * @return string|string[]
     */
    public function getTagsHeaderValue(array $tags);

So basically, it now allows an array of strings to be returned. This means all the existing formatters that return a string are just fine. Only the ones calling the method must also deal with an array return value. And this in fact should've always been the case because both, the PSR-7 Response::withHeader() as well as the Symfony $response->headers->set() accept an array to support multi-value headers. So, in theory this is a BC break but I don't think anyone will be affected.

Wdyt?

… formatter to a given size

dbu

i think the plan makes sense. i would be more relaxed to merge this if we can build a functional test with all caches that support tagging to prove that multiple tag headers will indeed work as expected (e.g. invalidation happens both if tag is in the first or in the second header)

i am a bit worried about the recursive splitting function you implemented. we potentially build several headers without then using them if they are too long. i think i would prefer to change the csv implementation to do that.

we already have a different solution to limit the header size when sending invalidation requests in the proxy clients:

FOSHttpCache/src/ProxyClient/Varnish.php

Lines 82 to 90 in 508d232

    
           $chunkSize = $this->determineTagsPerHeader($tags, $banMode ? '|' : ' '); 
        
           foreach (array_chunk($tags, $chunkSize) as $tagchunk) { 
        
               if ($banMode) { 
        
                   $this->invalidateByBan($tagchunk); 
        
               } else { 
        
                   $this->invalidateByPurgekeys($tagchunk); 
        
               } 
        
           }

and

FOSHttpCache/src/ProxyClient/Symfony.php

Lines 82 to 91 in 508d232

    
           $chunkSize = $this->determineTagsPerHeader($escapedTags, ','); 
        
           foreach (array_chunk($escapedTags, $chunkSize) as $tagchunk) { 
        
               $this->queueRequest( 
        
                   $this->options['tags_method'], 
        
                   '/', 
        
                   [$this->options['tags_header'] => implode(',', $tagchunk)], 
        
                   false 
        
               ); 
        
           }

dbu · 2018-08-11T07:17:02Z

tests/Unit/TagHeaderFormatter/MaxHeaderValueLengthFormatterTest.php

+            [3, ['foo', 'bar', 'baz'], ['foo', 'bar', 'baz']],
+            [6, ['foo', 'bar', 'baz'], ['foo', 'bar', 'baz']], // with the , that equals 7
+            [7, ['foo', 'bar', 'baz'], ['foo,bar', 'baz']],
+            [50, ['foo', 'bar', 'baz'], 'foo,bar,baz'], // long enough, must not be an array


why is it an array for 7 but not for 8+? that seems odd to me

Max length 6 is too short for foo,bar (7 chars) so it must result in two header values and 7 must result in one.

sorry, i missed the the line with 7 still has 2 elements. all fine with that then.

dbu · 2018-08-11T07:21:00Z

src/TagHeaderFormatter/MaxHeaderValueLengthFormatter.php

+    public function getTagsHeaderValue(array $tags)
+    {
+        $values = (array) $this->inner->getTagsHeaderValue($tags);
+        $newValues = [[]];


should this not be a single []?

Nope, this is because of array_merge(...$newValues);. You could also call array_merge() in every loop but it's a performance gain if you only merge it once :)

Toflar · 2018-08-13T11:09:42Z

i think the plan makes sense. i would be more relaxed to merge this if we can build a functional test with all caches that support tagging to prove that multiple tag headers will indeed work as expected (e.g. invalidation happens both if tag is in the first or in the second header)

As far as I can see, no proxy currently tests cache tag invalidation. I could only help with the SF proxy test anyway where I would need to add a dependency to my alternative PSR-6 store.

i am a bit worried about the recursive splitting function you implemented. we potentially build several headers without then using them if they are too long. i think i would prefer to change the csv implementation to do that.

I don't think this makes sense. The way I built it, the MaxHeaderValueLengthFormatter can be applied to any other formatter you like. No matter if it's CSV or anything else.
Also, let's be realistic here. It's so fast, it's just irrelevant. And chances that you actually hit the maximum are already low. Plus, even if you hit the maximum, chances that it needs multiple runs are really, really small because we split the whole tags array into halves.
I like that you think about these kind of things but I don't think there's anything to worry about here, really 😄

dbu · 2018-08-23T11:51:37Z

okay, agreed that the header should usually not need to be split many times.

and indeed, we miss functional tests for tagging. we do have purge/refresh/ban in
https://github.com/FriendsOfSymfony/FOSHttpCache/tree/master/tests/Functional/ProxyClient but miss tag tests. would be great if we could add them to be sure the multiple headers really work as expected. do you think you could add something for that? i guess they should go into a new test case in https://github.com/FriendsOfSymfony/FOSHttpCache/tree/master/tests/Functional/Symfony

can you please add something to the documentation for how to use this? and do you plan on adding this to the bundle afterwards?

…headers

Toflar · 2018-08-24T09:12:15Z

Pheeew, that was a hard nut to crack 😄 But I've added functional tests for cache tag invalidation in case of Symfony now and covered both cases, single and multiple headers. Of course that only works if my PSR-6 cache store is installed so I'm not sure if everything is going to work out fine on travis yet.

can you please add something to the documentation for how to use this?

Will do, once the PR is in ready-to-merge-state so we don't document stuff that's not true 😄

and do you plan on adding this to the bundle afterwards?

Yes I do.

Toflar · 2018-08-24T12:06:35Z

@dbu maybe you can help with the failing tests?

dbu · 2018-08-27T06:21:53Z

src/ProxyClient/Symfony.php

@@ -58,11 +58,13 @@ protected function configureOptions()
            'purge_method' => PurgeListener::DEFAULT_PURGE_METHOD,
            'tags_method' => PurgeTagsListener::DEFAULT_TAGS_METHOD,
            'tags_header' => PurgeTagsListener::DEFAULT_TAGS_HEADER,
+            'tags_path' => '/',


i agree with the feature, but lets call this tags_invalidate_path. its for the other thing than tags_header. (yes, we should have called tags_method tags_invalidate_method)

I agree here, changed in e6bfbf2.

dbu

great job! i restarted some things and now only the 2 builds with symfony 2.3 fail. that smelled like some sort of code rot, so i restarted the master build... but that build is still green. could it be that something changed with support for same-name headers since version 2 of symfony? if there is indeed a difference, i suggest we jump to support only symfony 3|4 in version 2.5 of the library. 2.7 is pretty much eol and does not even receive regular bug fixes anymore...

could you check if that is indeed the case and if so bump the branch alias in composer to 2.5 and change the minimum symfony requirement, and adjust .travis.yml?

Toflar · 2018-08-27T07:54:42Z

I've quickly tried to check but I would need to set up a docker container to replicate php 5.6 which is EOL very, very soon anyway too. Tbh, I would like to invest my time in more productive things than keeping support for versions nobody should be using anyway. If anybody really needs it, I think it should be them investing the time, not us 😄 Thus, I've bumped the minimum required versions.
One test is still failing but that looks like a time related thing which could be fixed by restarting the build.

Looks like this is going to make it so I'll be adding some docs soon.

dbu

cool! one change to composer.json, otherwise i think this is now ready. and the functional test indeed did reveal an issue, even if only with old symfony versions...

can you now add the documentation explaining the header splitting, and also a documentation entry for the tags_invalidate_path option?

@andrerom afaik ez was using an older symfony version. do you see a problem with this change? we can keep doing some bugfixing in the 2.4 version if needed, but i'd rather move to new versions as its an overhead to support several versions, but its also quite some overhead to support very old symfony versions...

dbu · 2018-08-27T07:55:58Z

composer.json

        "php-http/client-implementation": "^1.0.0",
        "php-http/client-common": "^1.1.0",
        "php-http/message": "^1.0.0",
-        "php-http/discovery": "^1.0"
+        "php-http/discovery": "^1.0",
+        "toflar/psr6-symfony-http-cache-store": "^1.1.2"


this line should not be here. better mark it in the conflicts section with < 1.1.2

Ah that was accidentally, fixed, sorry.

dbu · 2018-08-27T08:02:00Z

.travis.yml

-    - php: 7.2
-      env: DEPENDENCIES="symfony/lts:^2"
-    - php: 7.2
-      env: DEPENDENCIES="symfony/lts:^3 toflar/psr6-symfony-http-cache-store:^1.0"


should we not keep this build with lts 3?

As we've bumped to 3.4 as a minimum requirement I thought --prefer-lowest would already cover that as "lowest" === "3.4" now. Wrong?

i'd keep both so that we know if its only the oldest symfony 3.4 that fails or also the newest 3.4. (and also the other way round, we did have some regressions in the past, where some change in a patch version of old symfony broke stuff)

Okay, I'll bring it back then :)

dbu · 2018-08-27T08:04:15Z

the php 5.6 build with symfony 3.4 seems to work. agree that we should not waste too much time on supporting outdated things. people stuck on legacy can still use the 2.4 tag. i guess FOSHttpCache 3.0 should be PHP ^7.1 . (not in a hurry to do that, though)

Toflar · 2018-08-27T08:08:07Z

Docs ready for review.

dbu

doc looks good, suggested some tweaks.

can you please add an entry to the changelog, mentioning that the splitting is not active by default?

dbu · 2018-08-27T08:14:47Z

.travis.yml

@@ -7,7 +7,7 @@ cache:
 env:
  global:
    - VARNISH_VERSION=5.1
-    - DEPENDENCIES="toflar/psr6-symfony-http-cache-store:^1.0"
+    - DEPENDENCIES="toflar/psr6-symfony-http-cache-store:^1.1.2"


we should add a conflicts section to composer.json for older versions of the store, if the library does not work correctly with older versions. as this is an optional dependency, we can not control the version being installed otherwise.

dbu · 2018-08-27T08:16:53Z

doc/symfony-cache-configuration.rst

@@ -226,6 +226,10 @@ configure the HTTP method and header used for tag purging:

  **default**: ``X-Cache-Tags``

+* **tags_invalidate_path**: Path to where the invalidation request with the headers should be sent to.


i'd adjust this to what we have for tags_method. maybe:

"Path on the caching proxy to which the purge tags request should be sent."

dbu · 2018-08-27T08:19:05Z

doc/response-tagging.rst

@@ -56,6 +56,23 @@ empty tags::

    $responseTagger = new ResponseTagger(['strict' => true]);

+


i'd like to add a sub heading here to separate this a bit. "Working with large numbers of tags"?

dbu · 2018-08-27T08:22:50Z

doc/response-tagging.rst

+might get pretty long. In that case, again depending on your setup, you might run
+into server exceptions because the header value is too big to send. Mostly, this
+value seems to be about 4KB. The only thing you can do in such a case is to split
+up one header into multiple ones. Of course, your proxy then has to support multiple


i would take this out of the paragraph and make a .. note:: below the code example. and then mention that symfony httpcache and varnish do support multiple headers.

Toflar · 2018-08-27T08:36:19Z

Docs updated.

dbu · 2018-08-27T09:08:25Z

can you please add the changelog entry and make the part about the proxy having to support multiple headers a note?

the lowest version build failed, i restarted it to see if its a fluke. i wonder if those tests first check that we have a 2xx response first, or whether the real problem is some error from the server... we should probably output the response body when the assertion fails, to help debugging.

dbu · 2018-08-28T06:58:18Z

hah, now we see more. so on php 5.6 the toflar store is not found. which seems odd - do we not add it to the project?

the other tests have a problem with mockery and the getHeaders method...

dbu · 2018-08-28T07:00:19Z

tests/Functional/Fixtures/web/symfony.php


 $loader = require_once __DIR__.'/../../../../vendor/autoload.php';

 $symfonyProxy = new SymfonyProxy();

 $kernel = new AppKernel();
-$kernel = new AppCache($kernel, new Store($symfonyProxy->getCacheDir()), null, ['debug' => true]);
+$kernel = new AppCache($kernel, new Psr6Store(['cache_directory' => $symfonyProxy->getCacheDir(), 'cache_tags_header' => 'X-Cache-Tags']), null, ['debug' => true]);


the lowest version build clears the dependencies. i think we should only try to use the Psr6 store if class_exists and otherwise use the normal store.

andrerom · 2018-08-28T21:34:06Z

@andrerom afaik ez was using an older symfony version. do you see a problem with this change?

We are still on 1.x, started looking into moving to 2.x now that xkey is in, but it's invalidates most of our existing code so won't happen before later this fall, and that would be on our v2 (on PHP7+). Aka no blockers from us ;)

dbu

great job!

Toflar · 2018-08-29T09:25:33Z

Almost there, finishing the last issues with the tests :)

dbu · 2018-08-29T09:25:58Z

yep. why is the test for purge talking about the tags? do we have a wrong dependency here?

Toflar · 2018-08-29T09:26:36Z

No, no. I forgot half of it, I'm on it :)

Toflar · 2018-08-29T10:18:38Z

Wohooo 🎉

dbu · 2018-08-29T11:54:28Z

cheers!

do you want to work on the bundle integration before i tag the release? could be a nice further sanity check ;-)

Toflar · 2018-08-29T11:55:26Z

I will be but I'm on holidays for 3 weeks now so that'll take a while 😄 So no need to tag that right now. Thanks!

dbu · 2018-08-29T13:48:55Z

alright! enjoy your holidays then!

Description ----------- This needs to hold on for the releases. Related PR's: Library: * FriendsOfSymfony/FOSHttpCache#424 * FriendsOfSymfony/FOSHttpCache#426 Bundle: * FriendsOfSymfony/FOSHttpCache#426 Commits ------- f782ecc Configured max header value instead of removing the header altogether 1461dd0 Use 4096 instead of 2048

Description ----------- This needs to hold on for the releases. Related PR's: Library: * FriendsOfSymfony/FOSHttpCache#424 * FriendsOfSymfony/FOSHttpCache#426 Bundle: * FriendsOfSymfony/FOSHttpCache#426 Commits ------- f782ecc9 Configured max header value instead of removing the header altogether 1461dd09 Use 4096 instead of 2048

Toflar added 2 commits August 9, 2018 10:25

Added a TagHeaderFormatter that limits the header values from another…

f016240

… formatter to a given size

CS

bec93f3

dbu reviewed Aug 11, 2018

View reviewed changes

Added functional tests for tag invalidation with single and multiple …

897ca10

…headers

Toflar force-pushed the max-header-value-length-formatter branch from 16801a4 to 897ca10 Compare August 24, 2018 09:09

dbu reviewed Aug 27, 2018

View reviewed changes

Toflar added 2 commits August 27, 2018 09:30

Adjusted tags invalidation path configuration key

e6bfbf2

Dropped support for Symfony 2.*

8cab719

dbu reviewed Aug 27, 2018

View reviewed changes

Toflar added 3 commits August 27, 2018 10:04

Added docs

bcc091f

Removed composer entry

7932ab1

Documented tags_invalidate_path configuration option

da17114

Bring back the SF 3 LTS test

6702045

dbu reviewed Aug 27, 2018

View reviewed changes

Toflar added 2 commits August 27, 2018 10:32

Added a conflict for the psr-6 store

8034bb6

Improved docs

d6c710d

Toflar added 2 commits August 27, 2018 11:56

Added changelog entry

c6022b4

Improved docs once again

f44d4ee

Toflar force-pushed the max-header-value-length-formatter branch from f79faf2 to f455450 Compare August 27, 2018 10:06

Output more debugging info if cache constraint fails to assert

7be255b

Toflar force-pushed the max-header-value-length-formatter branch from f455450 to 7be255b Compare August 27, 2018 11:09

dbu reviewed Aug 28, 2018

View reviewed changes

Toflar added 2 commits August 29, 2018 11:00

Only test cache tagging if PSR-6 store is installed

8f38bd9

Fixed test constraint tests

7cfe604

dbu approved these changes Aug 29, 2018

View reviewed changes

Toflar added 2 commits August 29, 2018 11:27

Only add subscriber if store is loaded

c04ae47

Sometimes I wonder why I'm earning a living from coding :-)

67eacbd

dbu merged commit 6552fb1 into FriendsOfSymfony:master Aug 29, 2018

Toflar deleted the max-header-value-length-formatter branch August 29, 2018 11:55

dbu mentioned this pull request Aug 29, 2018

Integrate max header length formatter to split overlong tag headers in multiple FriendsOfSymfony/FOSHttpCacheBundle#481

Closed

This was referenced Sep 27, 2018

Added support for the new MaxHeaderValueLengthFormatter FriendsOfSymfony/FOSHttpCacheBundle#483

Merged

Configured max header value instead of removing the header altogether contao/contao#101

Merged

dbu mentioned this pull request May 6, 2019

Add a warning in documentation about memory consumption on varnish #281

Closed

	$chunkSize = $this->determineTagsPerHeader($tags, $banMode ? '\|' : ' ');

	foreach (array_chunk($tags, $chunkSize) as $tagchunk) {
	if ($banMode) {
	$this->invalidateByBan($tagchunk);
	} else {
	$this->invalidateByPurgekeys($tagchunk);
	}
	}

	$chunkSize = $this->determineTagsPerHeader($escapedTags, ',');

	foreach (array_chunk($escapedTags, $chunkSize) as $tagchunk) {
	$this->queueRequest(
	$this->options['tags_method'],
	'/',
	[$this->options['tags_header'] => implode(',', $tagchunk)],
	false
	);
	}

		@@ -226,6 +226,10 @@ configure the HTTP method and header used for tag purging:

		default: ``X-Cache-Tags``

		* tags_invalidate_path: Path to where the invalidation request with the headers should be sent to.

		@@ -56,6 +56,23 @@ empty tags::

		$responseTagger = new ResponseTagger(['strict' => true]);

[RFC] Added MaxHeaderValueLengthFormatter #424

[RFC] Added MaxHeaderValueLengthFormatter #424

Uh oh!

Conversation

Toflar commented Aug 9, 2018

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toflar commented Aug 13, 2018

Uh oh!

dbu commented Aug 23, 2018

Uh oh!

Toflar commented Aug 24, 2018

Uh oh!

Toflar commented Aug 24, 2018

Uh oh!

dbu Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Toflar commented Aug 27, 2018

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 27, 2018

Uh oh!

Toflar commented Aug 27, 2018

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toflar commented Aug 27, 2018

Uh oh!

dbu commented Aug 27, 2018

Uh oh!

dbu commented Aug 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dbu Aug 27, 2018 •

edited

Loading

andrerom commented Aug 28, 2018 •

edited

Loading