Skip to content

redis: use atomic operations everywhere #37758

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.

Sign up for GitHub

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
szaimen merged 3 commits into from
May 19, 2023
Merged

redis: use atomic operations everywhere #37758

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39e805f
redis: use atomic operations everywhere
pulsejet Apr 16, 2023
0375c98
add test to verify redis lua script hashes
icewind1991 Apr 17, 2023
c6cee28
redis: move lua scripts to class and add type hints
pulsejet May 9, 2023
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
Next Next commit
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 <[email protected]>
  • Loading branch information
@pulsejet
pulsejet committed Apr 16, 2023
commit 39e805fffadeb773bb186d695cf389cc0736a0f4
90 changes: 55 additions & 35 deletions lib/private/Memcache/Redis.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)) {
Copy link
Member Author

@pulsejet pulsejet Apr 16, 2023

Choose a reason for hiding this comment

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

This check is useless. Redis does not support boolean types, so getting a false means it did not exist.

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);
}
}

Expand All @@ -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);
Copy link
Member Author

@pulsejet pulsejet Apr 16, 2023

Choose a reason for hiding this comment

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

This will fail for redis cluster and a large number of keys. The solution is to replace this with a lua script and run it on all masters.

Expand All @@ -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);
}

/**
Expand All @@ -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;
}

/**
Expand All @@ -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();
Copy link
Member Author

@pulsejet pulsejet Apr 16, 2023

Choose a reason for hiding this comment

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

5 statements here vs 1 now. Ditto for cad.

return false;
$old = self::encodeValue($old);
$new = self::encodeValue($new);

return $this->evalLua('cas', [$key], [$old, $new]) > 0;
}

/**
Expand All @@ -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) {
Expand All @@ -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);
}
}