Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 31, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/Cache/Adapter/AbstractAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ abstract class AbstractAdapter implements AdapterInterface, LoggerAwareInterface
*/
protected function __construct($namespace = '', $defaultLifetime = 0)
{
$this->namespace = '' === $namespace ? '' : $this->getId($namespace).':';
$this->namespace = '' === $namespace ? '' : CacheItem::validateKey($namespace).':';
if (null !== $this->maxIdLength && strlen($namespace) > $this->maxIdLength - 24) {
throw new InvalidArgumentException(sprintf('Namespace must be %d chars max, %d given ("%s")', $this->maxIdLength - 24, strlen($namespace), $namespace));
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Cache/Adapter/ProxyAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function __construct(CacheItemPoolInterface $pool, $namespace = '', $defa
{
$this->pool = $pool;
$this->poolHash = $poolHash = spl_object_hash($pool);
$this->namespace = '' === $namespace ? '' : $this->getId($namespace);
$this->namespace = '' === $namespace ? '' : CacheItem::validateKey($namespace);
$this->namespaceLen = strlen($namespace);
$this->createCacheItem = \Closure::bind(
function ($key, $innerItem) use ($defaultLifetime, $poolHash) {
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/Cache/CacheItem.php
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ public function getPreviousTags()
*
* @param string $key The key to validate
*
* @return string
*
* @throws InvalidArgumentException When $key is not valid
*/
public static function validateKey($key)
Expand All @@ -161,6 +163,8 @@ public static function validateKey($key)
if (false !== strpbrk($key, '{}()/\@:')) {
throw new InvalidArgumentException(sprintf('Cache key "%s" contains reserved characters {}()/\@:', $key));
}

return $key;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Cache/Simple/AbstractCache.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ abstract class AbstractCache implements CacheInterface, LoggerAwareInterface
protected function __construct($namespace = '', $defaultLifetime = 0)
{
$this->defaultLifetime = max(0, (int) $defaultLifetime);
$this->namespace = '' === $namespace ? '' : $this->getId($namespace).':';
$this->namespace = '' === $namespace ? '' : CacheItem::validateKey($namespace).':';
if (null !== $this->maxIdLength && strlen($namespace) > $this->maxIdLength - 24) {
throw new InvalidArgumentException(sprintf('Namespace must be %d chars max, %d given ("%s")', $this->maxIdLength - 24, strlen($namespace), $namespace));
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Cache/Tests/CacheItemTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class CacheItemTest extends TestCase
{
public function testValidKey()
{
$this->assertNull(CacheItem::validateKey('foo'));
$this->assertSame('foo', CacheItem::validateKey('foo'));
}

/**
Expand Down
46 changes: 42 additions & 4 deletions src/Symfony/Component/Cache/Traits/AbstractTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ trait AbstractTrait
use LoggerAwareTrait;

private $namespace;
private $namespaceVersion = '';
private $versioningIsEnabled = false;
private $deferred = array();

/**
Expand Down Expand Up @@ -102,10 +104,18 @@ public function hasItem($key)
*/
public function clear()
{
if ($cleared = $this->versioningIsEnabled) {
$this->namespaceVersion = 2;
foreach ($this->doFetch(array('@'.$this->namespace)) as $v) {
$this->namespaceVersion = 1 + (int) $v;
}
$this->namespaceVersion .= ':';
$cleared = $this->doSave(array('@'.$this->namespace => $this->namespaceVersion), 0);
}
$this->deferred = array();

try {
return $this->doClear($this->namespace);
return $this->doClear($this->namespace) || $cleared;
} catch (\Exception $e) {
CacheItem::log($this->logger, 'Failed to clear the cache', array('exception' => $e));

Expand Down Expand Up @@ -158,6 +168,27 @@ public function deleteItems(array $keys)
return $ok;
}

/**
* Enables/disables versioning of items.
*
* 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.
Copy link
Contributor

@robfrawley robfrawley Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote memoize on purpose :)

*
* @param bool $enable
*
* @return bool the previous state of versioning
*/
public function enableVersioning($enable = true)
{
$wasEnabled = $this->versioningIsEnabled;
$this->versioningIsEnabled = (bool) $enable;
$this->namespaceVersion = '';

return $wasEnabled;
}

/**
* Like the native unserialize() function but throws an exception if anything goes wrong.
*
Expand Down Expand Up @@ -189,11 +220,18 @@ private function getId($key)
{
CacheItem::validateKey($key);

if ($this->versioningIsEnabled && '' === $this->namespaceVersion) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure it applies here

$this->namespaceVersion = '1:';
foreach ($this->doFetch(array('@'.$this->namespace)) as $v) {
$this->namespaceVersion = $v;
}
}

if (null === $this->maxIdLength) {
return $this->namespace.$key;
return $this->namespace.$this->namespaceVersion.$key;
}
if (strlen($id = $this->namespace.$key) > $this->maxIdLength) {
$id = $this->namespace.substr_replace(base64_encode(hash('sha256', $key, true)), ':', -22);
if (strlen($id = $this->namespace.$this->namespaceVersion.$key) > $this->maxIdLength) {
$id = $this->namespace.$this->namespaceVersion.substr_replace(base64_encode(hash('sha256', $key, true)), ':', -22);
}

return $id;
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Cache/Traits/MemcachedTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ private function init(\Memcached $client, $namespace, $defaultLifetime)
}

parent::__construct($namespace, $defaultLifetime);
$this->enableVersioning();
}

/**
Expand Down Expand Up @@ -242,7 +243,7 @@ protected function doDelete(array $ids)
*/
protected function doClear($namespace)
{
return $this->checkResultCode($this->getClient()->flush());
return false;
}

private function checkResultCode($result)
Expand Down
8 changes: 5 additions & 3 deletions src/Symfony/Component/Cache/Traits/RedisTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ public function init($redisClient, $namespace = '', $defaultLifetime = 0)
if (preg_match('#[^-+_.A-Za-z0-9]#', $namespace, $match)) {
throw new InvalidArgumentException(sprintf('RedisAdapter namespace contains "%s" but only characters in [-+_.A-Za-z0-9] are allowed.', $match[0]));
}
if (!$redisClient instanceof \Redis && !$redisClient instanceof \RedisArray && !$redisClient instanceof \RedisCluster && !$redisClient instanceof \Predis\Client) {
if ($redisClient instanceof \RedisCluster) {
$this->enableversioning();
} elseif (!$redisClient instanceof \Redis && !$redisClient instanceof \RedisArray && !$redisClient instanceof \Predis\Client) {
throw new InvalidArgumentException(sprintf('%s() expects parameter 1 to be Redis, RedisArray, RedisCluster or Predis\Client, %s given', __METHOD__, is_object($redisClient) ? get_class($redisClient) : gettype($redisClient)));
}
$this->redis = $redisClient;
Expand Down Expand Up @@ -171,8 +173,8 @@ protected function doHave($id)
*/
protected function doClear($namespace)
{
// When using a native Redis cluster, clearing the cache cannot work and always returns false.
// Clearing the cache should then be done by any other means (e.g. by restarting the cluster).
// When using a native Redis cluster, clearing the cache is done by versioning in AbstractTrait::clear().
// This means old keys are not really removed until they expire and may need gargage collection.

$cleared = true;
$hosts = array($this->redis);
Expand Down