lcache wildcard prefix clear removes all cache entries for the bin #1
Comments
|
I don't believe that will work correctly because a prefix wildcard clear isn't possible with LCache, so it escalates to a bin clear. |
|
@davidstrauss That is not correct, it would work fine with above patch. The bin clear is implemented as a prefix clear internally, so this should work fine. Edited: My original patch that comment was for was: - if ($wildcard) {
+ if ($wildcard && $cid == '*') {and that indeed did not work. |
|
I updated the patch. |
|
@davidstrauss Besides that full bin cache clears are a little problematic with Drupal out of the box: e.g. cache_bootstrap variable write would nuke your registry and module_implements, too. 'cache' cache_clear_all (e.g. entity_info_cache_clear) would nuke system_cache_tables and theme registry ... etc. |
|
@davidstrauss And its perfectly fine for APCu to not have the prefix clear, but do a bin clear. |
|
Hm, it seems that should work fine for APCu as well as that does use an iterator over the items. I am not seeing how a bin clear is different from a prefix clear given how its implemented. Do I miss something? |
|
Btw. Great work on lcache. Used it on an emergency black-friday project today and solved the problems with redis overload nicely. :) Besides the too much caches are cleared issue right now ... |
|
Okay, I see the StaticL2 cache makes an assumption on the bin. I need to fix Address::isMatch as well, so that it works correctly for the StaticL2. The rest looked good to me as they all use suffix '%' matching. |
|
And overall a 'bin' is as far as I can see an arbitrary concept in lcache. In the end its just the prefix of an address. So logically there should be no difference from a: 'cache_fabian' bin vs. a 'cache:fabian' bin if looking at it from the standpoint of an address. |
|
Indeed that might be another way to fix this easier. Just use the prefix as a 'bin', which lcache does not know anyway. |
|
Ah that won't work due to the bin prefix string. |
|
I would prefer to make the wildcard -> full bin conversion occur only in the Drupal module/include layer. It looks like the vendor directory got committed here, which is not correct. (I didn't set up the D7 GitHub repo but will fix it.) I would, instead, update the .inc file's factory for LCache\Address to set a wildcard to have a null $key. |
|
Yeah, need the wildcard support in Address class; just tested. |
|
@davidstrauss Any chance you can chat quickly on IRC / freenode.net? |
|
@davidstrauss The repo is fine, I am working on an emergency project, so committed vendor dir as a quick way to activate it. |
|
@davidstrauss So you mean to subclass the Address class and extend it just for the D7 module? |
|
Address is 'final' however ... |
|
All I'm suggesting is constructing an Address with a null $key whenever you need a wildcard. The only place the $wildcard attribute gets used is for the "entire bin" check, which can be triggered with a null $key. Anywhere else reading the key would be confused by a key with a wildcard, anyway. The bottom line is that I'm resistant to having Drupal's needs leak into LCache in this way. |
|
No need to subclass, just a factory that knows $wildcard===true means
coercing $key=null before constructing the Address.
As for IRC, I'm in the middle of a big board game with friends, but I have
some downtime between turns.
…On Sat, Nov 26, 2016, 16:41 Fabian Franz ***@***.***> wrote:
Address is 'final' however ...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAG6xtHfOhOY2aB9cWKh1qO5wB479iqLks5rCNGtgaJpZM4K9AGp>
.
|
|
i don't want a wildcard => full bin conversion. I want the opposite, I need wildcard / prefix cache clears working. I have patch for isMatch now, which is overly optimistic, but works fine: public function isMatch(Address $address)
{
if (!is_null($address->getBin()) && !is_null($this->bin) && $address->getBin() !== $this->bin) {
return false;
}
if ($address->isEntireBin() || $this->isEntireBin()) {
// @todo Check for wildcards.
return true;
}
if (!is_null($address->getKey()) && !is_null($this->key) && $address->getKey() !== $this->key) {
return false;
}
return true;
}e.g. if using StaticL2 it would see any prefix clear as a full bin clear, but not with any other cache backend that supports true prefixes. And yes it feels like a hack, but I need prefix clears working one way or another. |
|
@davidstrauss The module currently says that wildcache clears are full bin clears, but that is
|
|
It might be fine for the D7 module to have its own Address class with its own needs (wildcards). However, then it would be nice if upstream Address wasn't final as its a lot of code to copy, which defeats the purpose of the nice library. |
|
Yeah, I see the problem with my approach the $this->wildcard gets lost during serialization, which is used for L1Cache. |
|
Ah, I got it! :) The difference between a wildcard clear and a normal clear is a suffix of ":" per convention. And if no suffix is used, then I can still fallback to a normal full bin clear. diff --git a/sites/all/modules/contrib/lcache/lcache.cache.inc b/sites/all/modules/contrib/lcache/lcache.cache.inc
index c789454..602b7be 100644
--- a/sites/all/modules/contrib/lcache/lcache.cache.inc
+++ b/sites/all/modules/contrib/lcache/lcache.cache.inc
@@ -125,7 +125,7 @@ class LCache implements DrupalCacheInterface {
$this->integrated->collectGarbage();
}
else {
- if ($wildcard) {
+ if ($wildcard && ($cid == '*' || substr($cid, -1) !== ':')) {
$address = $this->getAddress();
$this->integrated->delete($address);
}
diff --git a/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php b/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php
index efe5d94..a49a513 100644
--- a/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php
+++ b/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php
@@ -50,7 +50,7 @@ final class Address implements \Serializable
*/
public function isEntireBin()
{
- return is_null($this->key);
+ return is_null($this->key) || substr($this->key, -1) === ':';
}
/**
@@ -72,7 +72,11 @@ final class Address implements \Serializable
{
if (!is_null($address->getBin()) && !is_null($this->bin) && $address->getBin() !== $this->bin) {
return false;
- }
+ }
+ if ($address->isEntireBin() || $this->isEntireBin()) {
+ // @todo Check for wildcards.
+ return true;
+ }
if (!is_null($address->getKey()) && !is_null($this->key) && $address->getKey() !== $this->key) {
return false;
}Of course this could be optimized to check for the prefix before the ':', too. e.g. for: foo:bar:baz if I try to clear: foo:bar I could fallback to clear: foo: instead easily. |
|
There's a lot of nuance to |
|
A serialized address uses the pattern |
diff --git a/sites/all/modules/contrib/lcache/lcache.cache.inc b/sites/all/modules/contrib/lcache/lcache.cache.inc
index c789454..5b3121e 100644
--- a/sites/all/modules/contrib/lcache/lcache.cache.inc
+++ b/sites/all/modules/contrib/lcache/lcache.cache.inc
@@ -63,8 +63,8 @@ class LCache implements DrupalCacheInterface {
$this->integrated = new \LCache\Integrated($l1, $l2, 100);
}
- protected function getAddress($cid=NULL) {
- return new \LCache\Address($this->bin, $cid);
+ protected function getAddress($cid=NULL, $is_prefix = FALSE) {
+ return new \LCache\Address($this->bin, $cid, $is_prefix);
}
/**
@@ -125,7 +125,7 @@ class LCache implements DrupalCacheInterface {
$this->integrated->collectGarbage();
}
else {
- if ($wildcard) {
+ if ($wildcard && $cid == '*') {
$address = $this->getAddress();
$this->integrated->delete($address);
}
@@ -136,7 +136,7 @@ class LCache implements DrupalCacheInterface {
}
}
else {
- $address = $this->getAddress($cid);
+ $address = $this->getAddress($cid, $wildcard);
$this->integrated->delete($address);
}
}
diff --git a/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php b/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php
index efe5d94..5dfffdb 100644
--- a/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php
+++ b/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/Address.php
@@ -12,18 +12,22 @@ final class Address implements \Serializable
protected $bin;
/** @var string|null */
protected $key;
+ /** @var bool */
+ protected $isPrefix = false;
/**
* Address constructor.
*
* @param string|null $bin
* @param string|null $key
+ * @param bool $prefix
*/
- public function __construct($bin = null, $key = null)
+ public function __construct($bin = null, $key = null, $is_prefix = false)
{
assert(!is_null($bin) || is_null($key));
$this->bin = $bin;
$this->key = $key;
+ $this->isPrefix = $is_prefix;
}
/**
@@ -50,7 +54,26 @@ final class Address implements \Serializable
*/
public function isEntireBin()
{
- return is_null($this->key);
+ return is_null($this->key) || $this->isPrefix();
+ }
+
+ public function isPrefix()
+ {
+ return $this->isPrefix;
+ }
+
+ public function serializePrefix()
+ {
+ if (is_null($this->bin)) {
+ return '';
+ }
+
+ $length_prefixed_bin = strlen($this->bin) . ':' . $this->bin;
+
+ if (is_null($this->key)) {
+ return $length_prefixed_bin . ':';
+ }
+ return $length_prefixed_bin . ':' . $this->key;
}
/**
@@ -72,7 +95,11 @@ final class Address implements \Serializable
{
if (!is_null($address->getBin()) && !is_null($this->bin) && $address->getBin() !== $this->bin) {
return false;
- }
+ }
+ if ($address->isPrefix() || $this->isPrefix()) {
+ // @todo Do real prefix check.
+ return true;
+ }
if (!is_null($address->getKey()) && !is_null($this->key) && $address->getKey() !== $this->key) {
return false;
}
@@ -98,7 +125,7 @@ final class Address implements \Serializable
$length_prefixed_bin = strlen($this->bin) . ':' . $this->bin;
- if (is_null($this->key)) {
+ if (is_null($this->key) || $this->isPrefix()) {
return $length_prefixed_bin . ':';
}
return $length_prefixed_bin . ':' . $this->key;
diff --git a/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/DatabaseL2.php b/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/DatabaseL2.php
index acffecd..84d9555 100644
--- a/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/DatabaseL2.php
+++ b/sites/all/modules/contrib/lcache/vendor/lcache/lcache/src/DatabaseL2.php
@@ -250,7 +250,12 @@ class DatabaseL2 extends L2
// Handle bin and larger deletions immediately. Queue individual key
// deletions for shutdown.
if ($address->isEntireBin() || $address->isEntireCache()) {
- $pattern = $address->serialize() . '%';
+ if ($address->isPrefix()) {
+ $pattern = $address->serializePrefix() . '%';
+ }
+ else {
+ $pattern = $address->serialize() . '%';
+ }
$sth = $this->dbh->prepare('DELETE FROM ' . $this->prefixTable('lcache_events') .' WHERE "event_id" < :new_event_id AND "address" LIKE :pattern');
$sth->bindValue(':new_event_id', $event_id, \PDO::PARAM_INT);
$sth->bindValue(':pattern', $pattern, \PDO::PARAM_STR); |
The text was updated successfully, but these errors were encountered: