From 39e805fffadeb773bb186d695cf389cc0736a0f4 Mon Sep 17 00:00:00 2001 From: Varun Patil Date: Sun, 16 Apr 2023 13:44:06 -0700 Subject: [PATCH 1/3] redis: use atomic operations everywhere This removes a lot of acrobatics in the code and does each operation atomically using a lua script. This also reduces several round trips to the server, and the scripts are compiled and cached server-side. Notably, since all operations work only on a single key (except clear, which is broken anyway and shouldn't be used), they will continue to function and be atomic for Redis cluster. Signed-off-by: Varun Patil --- lib/private/Memcache/Redis.php | 90 +++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 35 deletions(-) diff --git a/lib/private/Memcache/Redis.php b/lib/private/Memcache/Redis.php index b6f96fffba4eb..4c8151b9d6cd8 100644 --- a/lib/private/Memcache/Redis.php +++ b/lib/private/Memcache/Redis.php @@ -31,6 +31,22 @@ use OCP\IMemcacheTTL; +/** name => [script, sha1] */ +const LUA_SCRIPTS = [ + 'dec' => [ + 'if redis.call("exists", KEYS[1]) == 1 then return redis.call("decrby", KEYS[1], ARGV[1]) else return "NEX" end', + '720b40cb66cef1579f2ef16ec69b3da8c85510e9', + ], + 'cas' => [ + 'if redis.call("get", KEYS[1]) == ARGV[1] then redis.call("set", KEYS[1], ARGV[2]) return 1 else return 0 end', + '94eac401502554c02b811e3199baddde62d976d4', + ], + 'cad' => [ + 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end', + 'cf0e94b2e9ffc7e04395cf88f7583fc309985910', + ], +]; + class Redis extends Cache implements IMemcacheTTL { /** * @var \Redis|\RedisCluster $cache @@ -54,18 +70,19 @@ public function getCache() { public function get($key) { $result = $this->getCache()->get($this->getPrefix() . $key); - if ($result === false && !$this->getCache()->exists($this->getPrefix() . $key)) { + if ($result === false) { return null; - } else { - return json_decode($result, true); } + + return self::decodeValue($result); } public function set($key, $value, $ttl = 0) { + $value = self::encodeValue($value); if ($ttl > 0) { - return $this->getCache()->setex($this->getPrefix() . $key, $ttl, json_encode($value)); + return $this->getCache()->setex($this->getPrefix() . $key, $ttl, $value); } else { - return $this->getCache()->set($this->getPrefix() . $key, json_encode($value)); + return $this->getCache()->set($this->getPrefix() . $key, $value); } } @@ -82,6 +99,7 @@ public function remove($key) { } public function clear($prefix = '') { + // TODO: this is slow and would fail with Redis cluster $prefix = $this->getPrefix() . $prefix . '*'; $keys = $this->getCache()->keys($prefix); $deleted = $this->getCache()->del($keys); @@ -98,17 +116,14 @@ public function clear($prefix = '') { * @return bool */ public function add($key, $value, $ttl = 0) { - // don't encode ints for inc/dec - if (!is_int($value)) { - $value = json_encode($value); - } + $value = self::encodeValue($value); $args = ['nx']; if ($ttl !== 0 && is_int($ttl)) { $args['ex'] = $ttl; } - return $this->getCache()->set($this->getPrefix() . $key, (string)$value, $args); + return $this->getCache()->set($this->getPrefix() . $key, $value, $args); } /** @@ -130,10 +145,8 @@ public function inc($key, $step = 1) { * @return int | bool */ public function dec($key, $step = 1) { - if (!$this->hasKey($key)) { - return false; - } - return $this->getCache()->decrBy($this->getPrefix() . $key, $step); + $res = $this->evalLua('dec', [$key], [$step]); + return ($res === 'NEX') ? false : $res; } /** @@ -145,18 +158,10 @@ public function dec($key, $step = 1) { * @return bool */ public function cas($key, $old, $new) { - if (!is_int($new)) { - $new = json_encode($new); - } - $this->getCache()->watch($this->getPrefix() . $key); - if ($this->get($key) === $old) { - $result = $this->getCache()->multi() - ->set($this->getPrefix() . $key, $new) - ->exec(); - return $result !== false; - } - $this->getCache()->unwatch(); - return false; + $old = self::encodeValue($old); + $new = self::encodeValue($new); + + return $this->evalLua('cas', [$key], [$old, $new]) > 0; } /** @@ -167,15 +172,9 @@ public function cas($key, $old, $new) { * @return bool */ public function cad($key, $old) { - $this->getCache()->watch($this->getPrefix() . $key); - if ($this->get($key) === $old) { - $result = $this->getCache()->multi() - ->unlink($this->getPrefix() . $key) - ->exec(); - return $result !== false; - } - $this->getCache()->unwatch(); - return false; + $old = self::encodeValue($old); + + return $this->evalLua('cad', [$key], [$old]) > 0; } public function setTTL($key, $ttl) { @@ -185,4 +184,25 @@ public function setTTL($key, $ttl) { public static function isAvailable(): bool { return \OC::$server->getGetRedisFactory()->isAvailable(); } + + protected function evalLua($scriptName, $keys, $args) { + $keys = array_map(fn ($key) => $this->getPrefix() . $key, $keys); + $args = array_merge($keys, $args); + $script = LUA_SCRIPTS[$scriptName]; + + $result = $this->getCache()->evalSha($script[1], $args, count($keys)); + if (false === $result) { + $result = $this->getCache()->eval($script[0], $args, count($keys)); + } + + return $result; + } + + protected static function encodeValue($value) { + return is_int($value) ? (string) $value : json_encode($value); + } + + protected static function decodeValue($value) { + return is_numeric($value) ? (int) $value : json_decode($value, true); + } } From 0375c9866c12b9698e60ac021aae7de591dc7d57 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 17 Apr 2023 16:18:48 +0200 Subject: [PATCH 2/3] add test to verify redis lua script hashes Signed-off-by: Robin Appelman --- tests/lib/Memcache/RedisTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/lib/Memcache/RedisTest.php b/tests/lib/Memcache/RedisTest.php index 276dbf3a550cc..9e40698ec068b 100644 --- a/tests/lib/Memcache/RedisTest.php +++ b/tests/lib/Memcache/RedisTest.php @@ -9,6 +9,8 @@ namespace Test\Memcache; +use const OC\Memcache\LUA_SCRIPTS; + /** * @group Memcache * @group Redis @@ -56,4 +58,10 @@ protected function setUp(): void { parent::setUp(); $this->instance = new \OC\Memcache\Redis($this->getUniqueID()); } + + public function testScriptHashes() { + foreach (LUA_SCRIPTS as $script) { + $this->assertEquals(sha1($script[0]), $script[1]); + } + } } From c6cee282b345842f30afc52478d7b30696bed29d Mon Sep 17 00:00:00 2001 From: Varun Patil Date: Tue, 9 May 2023 11:04:32 -0700 Subject: [PATCH 3/3] redis: move lua scripts to class and add type hints Signed-off-by: Varun Patil --- lib/private/Memcache/Redis.php | 40 ++++++++++++++++---------------- tests/lib/Memcache/RedisTest.php | 4 +--- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/lib/private/Memcache/Redis.php b/lib/private/Memcache/Redis.php index 4c8151b9d6cd8..bde25a3385a93 100644 --- a/lib/private/Memcache/Redis.php +++ b/lib/private/Memcache/Redis.php @@ -31,23 +31,23 @@ use OCP\IMemcacheTTL; -/** name => [script, sha1] */ -const LUA_SCRIPTS = [ - 'dec' => [ - 'if redis.call("exists", KEYS[1]) == 1 then return redis.call("decrby", KEYS[1], ARGV[1]) else return "NEX" end', - '720b40cb66cef1579f2ef16ec69b3da8c85510e9', - ], - 'cas' => [ - 'if redis.call("get", KEYS[1]) == ARGV[1] then redis.call("set", KEYS[1], ARGV[2]) return 1 else return 0 end', - '94eac401502554c02b811e3199baddde62d976d4', - ], - 'cad' => [ - 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end', - 'cf0e94b2e9ffc7e04395cf88f7583fc309985910', - ], -]; - class Redis extends Cache implements IMemcacheTTL { + /** name => [script, sha1] */ + public const LUA_SCRIPTS = [ + 'dec' => [ + 'if redis.call("exists", KEYS[1]) == 1 then return redis.call("decrby", KEYS[1], ARGV[1]) else return "NEX" end', + '720b40cb66cef1579f2ef16ec69b3da8c85510e9', + ], + 'cas' => [ + 'if redis.call("get", KEYS[1]) == ARGV[1] then redis.call("set", KEYS[1], ARGV[2]) return 1 else return 0 end', + '94eac401502554c02b811e3199baddde62d976d4', + ], + 'cad' => [ + 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end', + 'cf0e94b2e9ffc7e04395cf88f7583fc309985910', + ], + ]; + /** * @var \Redis|\RedisCluster $cache */ @@ -185,10 +185,10 @@ public static function isAvailable(): bool { return \OC::$server->getGetRedisFactory()->isAvailable(); } - protected function evalLua($scriptName, $keys, $args) { + protected function evalLua(string $scriptName, array $keys, array $args) { $keys = array_map(fn ($key) => $this->getPrefix() . $key, $keys); $args = array_merge($keys, $args); - $script = LUA_SCRIPTS[$scriptName]; + $script = self::LUA_SCRIPTS[$scriptName]; $result = $this->getCache()->evalSha($script[1], $args, count($keys)); if (false === $result) { @@ -198,11 +198,11 @@ protected function evalLua($scriptName, $keys, $args) { return $result; } - protected static function encodeValue($value) { + protected static function encodeValue(mixed $value): string { return is_int($value) ? (string) $value : json_encode($value); } - protected static function decodeValue($value) { + protected static function decodeValue(string $value): mixed { return is_numeric($value) ? (int) $value : json_decode($value, true); } } diff --git a/tests/lib/Memcache/RedisTest.php b/tests/lib/Memcache/RedisTest.php index 9e40698ec068b..b94b69a5e6ac9 100644 --- a/tests/lib/Memcache/RedisTest.php +++ b/tests/lib/Memcache/RedisTest.php @@ -9,8 +9,6 @@ namespace Test\Memcache; -use const OC\Memcache\LUA_SCRIPTS; - /** * @group Memcache * @group Redis @@ -60,7 +58,7 @@ protected function setUp(): void { } public function testScriptHashes() { - foreach (LUA_SCRIPTS as $script) { + foreach (\OC\Memcache\Redis::LUA_SCRIPTS as $script) { $this->assertEquals(sha1($script[0]), $script[1]); } }