[Cache] Use namespace versioning for backends that dont support clearing by keys #23969

nicolas-grekas · 2017-08-24T06:40:06Z

Q	A
Branch?	3.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23960
License	MIT
Doc PR	-

jderusse · 2017-08-24T07:10:30Z

src/Symfony/Component/Cache/Traits/AbstractTrait.php

@@ -189,11 +204,18 @@ private function getId($key)
    {
        CacheItem::validateKey($key);

+        if ($this->versioningIsEnabled && '' === $this->namespaceVersion) {


Other classes used the trick isset($string[0]) to check if their is a namespace.

not sure it applies here

nicolas-grekas · 2017-08-24T07:17:23Z

with new false positive from php-cs-fixer...

hcomnetworkers

Looks good!

Although I'm wondering why you put a colon after the namespace while colons are not allowed in validateKey?
(We used to use colons in our cache keys until we switched to the new classes which don't allow it ;)
Also on a side note: Is there any reason why an integer is not allowed as cache key if it gets namespaced anyway? We have/had that, unfortunately.)

nicolas-grekas · 2017-08-24T11:34:00Z

put a colon after the namespace while colons are not allowed in validateKey

I put a colon because they are not allowed when given from the outside. Here, we're past the interface, so the PSR-6/16 constraints do not apply. Which is very nice for us: using a colon makes it impossible to have any confusion with user input.

why an integer is not allowed as cache key

because PSR-6/16 states that a key must be a string. So 6 is not allowed, but "6" is.

andrerom · 2017-08-26T15:12:06Z

I'm a bit torn on this, as it adds extra lookup for every lookup. Would it be possible to rather do this on first usage, and afterwards get it using multi get together with the item lookup? Then if version has changed either refetch the items, or in future when there is stale cache support on get we can still return the items when callee indicated it does not need fresh items, however have the new version set for next caller.

If that won't fly, then at least make sure this is only done once on multi get lookus. in current patch here it will be done for every key before lookup on all is done, which I would consider a bug.

nicolas-grekas · 2017-08-26T15:27:14Z

As the PR is now, the version retrieval is done only once, on first need. That's how Doctrine does it also afaik. Isn't it good enough?

andrerom · 2017-08-26T15:32:33Z

Ah, sorry missed that completely. It's fine with me, but you can bet there will be bug report for this down the line for long running processes :)

…ing by keys

nicolas-grekas · 2017-08-27T08:04:28Z

there will be bug report for this down the line for long running processes

so, I added a public method to explicitly reset the memoized version, for the long running process use case. This looks much more reasonable to me than fetching the key on every fetch, as even multi-fetch are not always grouped (eg RedisCluster or RedisArray), thus would pay an extra cost at each fetch.

This requires long running processes to do something like that in their main loop:
$pool->enableVersioning() || $pool->enableVersioning(false);

Yes, this is a "new feature" (because a new public method is). Yet, it is required to compensate for a behavior change (long running process don't have their cache cleared anymore). And the behavior change is also technically a BC break. Yet also I think this is a really edge-case one and the issue is at the design level so that it cannot be solved otherwise.

jderusse · 2017-08-27T08:36:40Z

A use case for such each case would be to implement a reverse proxy base on ReactPHP (php-pm for instance). With cache invalidation with the clear method.
(I'm not sur it exists and it'd a good idea.. just a brainstorm idea of what could be wrong)

andrerom · 2017-08-27T08:42:47Z

Yes, ReactPHP or similar will hit this. But I agree with @nicolas-grekas that main usage should not pay a penalty because of it. One option would be to clearly document handlers/ connection classes that don't support flushing pool and hence not recommended for use with long running php processes such as ReactPHP/cron/script.

nicolas-grekas · 2017-08-27T08:57:39Z

to clearly document handlers/ connection classes that don't support flushing

well, with this patch, all handlers do support flushing. But for long running processes, the concern leaks, and I have no better reasonable idea to make it not.

So, we would require ppl to call $pool->enableVersioning() on pools that have versioning enabled.
(or call $pool->enableVersioning() || $pool->enableVersioning(false), which works whether versioning is on or not, without changing the versioning-enabled state.)

fabpot · 2017-08-31T21:56:20Z

Thank you @nicolas-grekas.

…pport clearing by keys (nicolas-grekas) This PR was merged into the 3.3 branch. Discussion ---------- [Cache] Use namespace versioning for backends that dont support clearing by keys | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23960 | License | MIT | Doc PR | - Commits ------- f8a7518 [Cache] Use namespace versioning for backends that dont support clearing by keys

robfrawley · 2017-09-01T07:35:10Z

src/Symfony/Component/Cache/Traits/AbstractTrait.php

+     * When versioning is enabled, clearing the cache is atomic and doesn't require listing existing keys to proceed,
+     * but old keys may need garbage collection and extra round-trips to the back-end are required.
+     *
+     * Calling this method also clears the memoized namespace version and thus forces a resynchonization of it.


@nicolas-grekas Spelling errors: "memoized" -> "memorized" and "resynchonization" -> "resynchronization"

I reported the "memoized" typo once ... and I was wrong :( "Memoize" seems to be a valid concept: https://en.wikipedia.org/wiki/Memoization Maybe it's used correctly this time too?

Ah; seems you are correct! I think the latter one is a spelling error though. ;-)

I wrote memoize on purpose :)

…k component (jderusse) This PR was submitted for the 3.4 branch but it was merged into the 3.3 branch instead (closes #23958). Discussion ---------- [Lock] Fix race condition in tests between cache and lock component | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Currently trying to fix * [x] php 5.5 in testSaveWithDifferentResources "Failed asserting that false is true" * [x] php 5.5 in testSaveWithDifferentKeysOnSameResources "The store shouldn't save the second key" Workflow: * [x] find a reproducer * [x] fix memcached tests => #23969 * [x] fix redis tests => this PR Commits ------- 26948cf Fix race condition in tests between cache and lock

nicolas-grekas added this to the 3.3 milestone Aug 24, 2017

carsonbot added Status: Needs Review Cache Bug labels Aug 24, 2017

nicolas-grekas changed the title ~~[Cache] Use namespace versioning for backends that dont support clear…~~ [Cache] Use namespace versioning for backends that dont support clearing by keys Aug 24, 2017

nicolas-grekas force-pushed the cache-version branch 3 times, most recently from d33fc54 to c15d72e Compare August 24, 2017 06:55

nicolas-grekas mentioned this pull request Aug 24, 2017

Make AbstractTrait::getId and $namespace protected. #23960

Closed

jderusse reviewed Aug 24, 2017

View reviewed changes

nicolas-grekas force-pushed the cache-version branch from c15d72e to 63b5154 Compare August 24, 2017 07:15

nicolas-grekas force-pushed the cache-version branch from 63b5154 to 025be26 Compare August 24, 2017 07:54

jderusse mentioned this pull request Aug 24, 2017

[Lock] Fix race condition in tests between cache and lock component #23958

Closed

5 tasks

hcomnetworkers approved these changes Aug 24, 2017

View reviewed changes

nicolas-grekas force-pushed the cache-version branch from 025be26 to b245adc Compare August 24, 2017 11:36

[Cache] Use namespace versioning for backends that dont support clear…

f8a7518

…ing by keys

nicolas-grekas force-pushed the cache-version branch from b245adc to f8a7518 Compare August 27, 2017 07:57

fabpot approved these changes Aug 31, 2017

View reviewed changes

fabpot merged commit f8a7518 into symfony:3.3 Aug 31, 2017

nicolas-grekas deleted the cache-version branch September 1, 2017 06:57

robfrawley reviewed Sep 1, 2017

View reviewed changes

fabpot mentioned this pull request Sep 11, 2017

Release v3.3.9 #24156

Merged

Uh oh!

[Cache] Use namespace versioning for backends that dont support clearing by keys #23969

[Cache] Use namespace versioning for backends that dont support clearing by keys #23969

Uh oh!

Conversation

nicolas-grekas commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jderusse Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Aug 24, 2017

Uh oh!

hcomnetworkers left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

nicolas-grekas commented Aug 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

nicolas-grekas commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jderusse commented Aug 27, 2017

Uh oh!

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

Uh oh!

Uh oh!

nicolas-grekas commented Aug 27, 2017

Uh oh!

fabpot commented Aug 31, 2017

Uh oh!

robfrawley Sep 1, 2017 • 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!

javiereguiluz Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

robfrawley Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolas-grekas commented Aug 24, 2017 •

edited

Loading

nicolas-grekas commented Aug 24, 2017 •

edited

Loading

andrerom commented Aug 26, 2017 •

edited

Loading

andrerom commented Aug 26, 2017 •

edited

Loading

nicolas-grekas commented Aug 27, 2017 •

edited

Loading

andrerom commented Aug 27, 2017 •

edited

Loading

robfrawley Sep 1, 2017 •

edited

Loading