From 33918821f12d1f63b4c45080a3b3977acd39d4c3 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 29 Mar 2023 18:03:03 +0200 Subject: [PATCH 1/2] restore shared lock ttl when releasing Signed-off-by: Robin Appelman --- lib/private/Lock/MemcacheLockingProvider.php | 80 ++++++++++++++++++- lib/private/Memcache/LoggerWrapperCache.php | 10 ++- lib/private/Memcache/ProfilerWrapperCache.php | 10 ++- lib/private/Memcache/Redis.php | 17 ++++ lib/private/Server.php | 4 +- lib/public/IMemcacheTTL.php | 19 ++++- .../lib/Lock/MemcacheLockingProviderTest.php | 4 +- 7 files changed, 136 insertions(+), 8 deletions(-) diff --git a/lib/private/Lock/MemcacheLockingProvider.php b/lib/private/Lock/MemcacheLockingProvider.php index d4eebd7c30221..720105705996b 100644 --- a/lib/private/Lock/MemcacheLockingProvider.php +++ b/lib/private/Lock/MemcacheLockingProvider.php @@ -27,21 +27,42 @@ */ namespace OC\Lock; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\IMemcache; use OCP\IMemcacheTTL; use OCP\Lock\LockedException; class MemcacheLockingProvider extends AbstractLockingProvider { + /** @var array */ + private array $oldTTLs = []; private IMemcache $memcache; + private ITimeFactory $timeFactory; - public function __construct(IMemcache $memcache, int $ttl = 3600) { + public function __construct(IMemcache $memcache, ITimeFactory $timeFactory, int $ttl = 3600) { $this->memcache = $memcache; + $this->timeFactory = $timeFactory; $this->ttl = $ttl; } - private function setTTL(string $path): void { + private function setTTL(string $path, int $ttl = null, ?int $compare = null): void { + if (is_null($ttl)) { + $ttl = $this->ttl; + } + if ($this->memcache instanceof IMemcacheTTL) { + if ($compare !== null) { + $this->memcache->compareSetTTL($path, $compare, $ttl); + } else { + $this->memcache->setTTL($path, $ttl); + } + } + } + + private function getTTL(string $path): int { if ($this->memcache instanceof IMemcacheTTL) { - $this->memcache->setTTL($path, $this->ttl); + $ttl = $this->memcache->getTTL($path); + return $ttl === false ? -1 : $ttl; + } else { + return -1; } } @@ -58,14 +79,22 @@ public function isLocked(string $path, int $type): bool { public function acquireLock(string $path, int $type, ?string $readablePath = null): void { if ($type === self::LOCK_SHARED) { + // save the old TTL to for `restoreTTL` + $this->oldTTLs[$path] = [ + "ttl" => $this->getTTL($path), + "time" => $this->timeFactory->getTime() + ]; if (!$this->memcache->inc($path)) { throw new LockedException($path, null, $this->getExistingLockForException($path), $readablePath); } } else { + // when getting exclusive locks, we know there are no old TTLs to restore $this->memcache->add($path, 0); + // ttl is updated automatically when the `set` succeeds if (!$this->memcache->cas($path, 0, 'exclusive')) { throw new LockedException($path, null, $this->getExistingLockForException($path), $readablePath); } + unset($this->oldTTLs[$path]); } $this->setTTL($path); $this->markAcquire($path, $type); @@ -88,6 +117,12 @@ public function releaseLock(string $path, int $type): void { $newValue = $this->memcache->dec($path); } + if ($newValue > 0) { + $this->restoreTTL($path); + } else { + unset($this->oldTTLs[$path]); + } + // if we somehow release more locks then exists, reset the lock if ($newValue < 0) { $this->memcache->cad($path, $newValue); @@ -106,13 +141,52 @@ public function changeLock(string $path, int $targetType): void { } elseif ($targetType === self::LOCK_EXCLUSIVE) { // we can only change a shared lock to an exclusive if there's only a single owner of the shared lock if (!$this->memcache->cas($path, 1, 'exclusive')) { + $this->restoreTTL($path); throw new LockedException($path, null, $this->getExistingLockForException($path)); } + unset($this->oldTTLs[$path]); } $this->setTTL($path); $this->markChange($path, $targetType); } + /** + * With shared locks, each time the lock is acquired, the ttl for the path is reset. + * + * Due to this "ttl extension" when a shared lock isn't freed correctly for any reason + * the lock won't expire until no shared locks are required for the path for 1h. + * This can lead to a client repeatedly trying to upload a file, and failing forever + * because the lock never gets the opportunity to expire. + * + * To help the lock expire in this case, we lower the TTL back to what it was before we + * took the shared lock *only* if nobody else got a shared lock after we did. + * + * This doesn't handle all cases where multiple requests are acquiring shared locks + * but it should handle some of the more common ones and not hurt things further + */ + private function restoreTTL(string $path): void { + if (isset($this->oldTTLs[$path])) { + $saved = $this->oldTTLs[$path]; + $elapsed = $this->timeFactory->getTime() - $saved['time']; + + // old value to compare to when setting ttl in case someone else changes the lock in the middle of this function + $value = $this->memcache->get($path); + + $currentTtl = $this->getTTL($path); + + // what the old ttl would be given the time elapsed since we acquired the lock + // note that if this gets negative the key will be expired directly when we set the ttl + $remainingOldTtl = $saved['ttl'] - $elapsed; + // what the currently ttl would be if nobody else acquired a lock since we did (+1 to cover rounding errors) + $expectedTtl = $this->ttl - $elapsed + 1; + + // check if another request has acquired a lock (and didn't release it yet) + if ($currentTtl <= $expectedTtl) { + $this->setTTL($path, $remainingOldTtl, $value); + } + } + } + private function getExistingLockForException(string $path): string { $existing = $this->memcache->get($path); if (!$existing) { diff --git a/lib/private/Memcache/LoggerWrapperCache.php b/lib/private/Memcache/LoggerWrapperCache.php index 55c0e76db792c..55d5104dedba3 100644 --- a/lib/private/Memcache/LoggerWrapperCache.php +++ b/lib/private/Memcache/LoggerWrapperCache.php @@ -167,10 +167,18 @@ public function cad($key, $old) { } /** @inheritDoc */ - public function setTTL($key, $ttl) { + public function setTTL(string $key, int $ttl) { $this->wrappedCache->setTTL($key, $ttl); } + public function getTTL(string $key): int|false { + return $this->wrappedCache->getTTL($key); + } + + public function compareSetTTL(string $key, mixed $value, int $ttl): bool { + return $this->wrappedCache->compareSetTTL($key, $value, $ttl); + } + public static function isAvailable(): bool { return true; } diff --git a/lib/private/Memcache/ProfilerWrapperCache.php b/lib/private/Memcache/ProfilerWrapperCache.php index 6e76989ddddea..a5cb667114cc9 100644 --- a/lib/private/Memcache/ProfilerWrapperCache.php +++ b/lib/private/Memcache/ProfilerWrapperCache.php @@ -183,10 +183,18 @@ public function cad($key, $old) { } /** @inheritDoc */ - public function setTTL($key, $ttl) { + public function setTTL(string $key, int $ttl) { $this->wrappedCache->setTTL($key, $ttl); } + public function getTTL(string $key): int|false { + return $this->wrappedCache->getTTL($key); + } + + public function compareSetTTL(string $key, mixed $value, int $ttl): bool { + return $this->wrappedCache->compareSetTTL($key, $value, $ttl); + } + public function offsetExists($offset): bool { return $this->hasKey($offset); } diff --git a/lib/private/Memcache/Redis.php b/lib/private/Memcache/Redis.php index 38cee3ef6cb7c..9912063c99a4b 100644 --- a/lib/private/Memcache/Redis.php +++ b/lib/private/Memcache/Redis.php @@ -181,6 +181,23 @@ public function setTTL($key, $ttl) { $this->getCache()->expire($this->getPrefix() . $key, $ttl); } + public function getTTL(string $key): int|false { + $ttl = $this->getCache()->ttl($this->getPrefix() . $key); + return $ttl > 0 ? (int)$ttl : false; + } + + public function compareSetTTL(string $key, mixed $value, int $ttl): bool { + $fullKey = $this->getPrefix() . $key; + $redis = $this->getCache(); + if ($this->get($key) === $value) { + $result = $redis->multi() + ->expire($fullKey, $ttl) + ->exec(); + return $result !== false; + } + return false; + } + public static function isAvailable(): bool { return \OC::$server->getGetRedisFactory()->isAvailable(); } diff --git a/lib/private/Server.php b/lib/private/Server.php index ea62bf8324bf6..008019db97631 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -164,6 +164,7 @@ use OCA\Theming\Util; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Authentication\LoginCredentials\IStore; use OCP\Authentication\Token\IProvider as OCPIProvider; use OCP\BackgroundJob\IJobList; @@ -1115,7 +1116,8 @@ public function __construct($webRoot, \OC\Config $config) { $memcacheFactory = $c->get(ICacheFactory::class); $memcache = $memcacheFactory->createLocking('lock'); if (!($memcache instanceof \OC\Memcache\NullCache)) { - return new MemcacheLockingProvider($memcache, $ttl); + $timeFactory = $c->get(ITimeFactory::class); + return new MemcacheLockingProvider($memcache, $timeFactory, $ttl); } return new DBLockingProvider( $c->get(IDBConnection::class), diff --git a/lib/public/IMemcacheTTL.php b/lib/public/IMemcacheTTL.php index 8424e5b2bfa7f..250e727308b2c 100644 --- a/lib/public/IMemcacheTTL.php +++ b/lib/public/IMemcacheTTL.php @@ -35,5 +35,22 @@ interface IMemcacheTTL extends IMemcache { * @param int $ttl time to live in seconds * @since 8.2.2 */ - public function setTTL($key, $ttl); + public function setTTL(string $key, int $ttl); + + /** + * Get the ttl for an existing value, in seconds till expiry + * + * @return int|false + * @since 27 + */ + public function getTTL(string $key): int|false; + /** + * Set the ttl for an existing value if the value matches + * + * @param string $key + * @param mixed $value + * @param int $ttl time to live in seconds + * @since 27 + */ + public function compareSetTTL(string $key, $value, int $ttl): bool; } diff --git a/tests/lib/Lock/MemcacheLockingProviderTest.php b/tests/lib/Lock/MemcacheLockingProviderTest.php index b67be799d59db..95001ec03a306 100644 --- a/tests/lib/Lock/MemcacheLockingProviderTest.php +++ b/tests/lib/Lock/MemcacheLockingProviderTest.php @@ -22,6 +22,7 @@ namespace Test\Lock; use OC\Memcache\ArrayCache; +use OCP\AppFramework\Utility\ITimeFactory; class MemcacheLockingProviderTest extends LockingProvider { /** @@ -34,7 +35,8 @@ class MemcacheLockingProviderTest extends LockingProvider { */ protected function getInstance() { $this->memcache = new ArrayCache(); - return new \OC\Lock\MemcacheLockingProvider($this->memcache); + $timeProvider = \OC::$server->get(ITimeFactory::class); + return new \OC\Lock\MemcacheLockingProvider($this->memcache, $timeProvider); } protected function tearDown(): void { From a59df23fd5f26a3deb0cbc4347c569ab920bd705 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 5 Dec 2023 16:01:46 +0100 Subject: [PATCH 2/2] adjust redis compareSetTTL to use a lua script Signed-off-by: Robin Appelman --- lib/private/Memcache/Redis.php | 16 +++++++--------- tests/lib/Memcache/RedisTest.php | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/lib/private/Memcache/Redis.php b/lib/private/Memcache/Redis.php index 9912063c99a4b..953ec93c5275a 100644 --- a/lib/private/Memcache/Redis.php +++ b/lib/private/Memcache/Redis.php @@ -46,6 +46,10 @@ class Redis extends Cache implements IMemcacheTTL { 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end', 'cf0e94b2e9ffc7e04395cf88f7583fc309985910', ], + 'caSetTtl' => [ + 'if redis.call("get", KEYS[1]) == ARGV[1] then redis.call("expire", KEYS[1], ARGV[2]) return 1 else return 0 end', + 'fa4acbc946d23ef41d7d3910880b60e6e4972d72', + ], ]; /** @@ -187,15 +191,9 @@ public function getTTL(string $key): int|false { } public function compareSetTTL(string $key, mixed $value, int $ttl): bool { - $fullKey = $this->getPrefix() . $key; - $redis = $this->getCache(); - if ($this->get($key) === $value) { - $result = $redis->multi() - ->expire($fullKey, $ttl) - ->exec(); - return $result !== false; - } - return false; + $value = self::encodeValue($value); + + return $this->evalLua('caSetTtl', [$key], [$value, $ttl]) > 0; } public static function isAvailable(): bool { diff --git a/tests/lib/Memcache/RedisTest.php b/tests/lib/Memcache/RedisTest.php index b94b69a5e6ac9..27c6fc11ee86a 100644 --- a/tests/lib/Memcache/RedisTest.php +++ b/tests/lib/Memcache/RedisTest.php @@ -9,11 +9,18 @@ namespace Test\Memcache; +use OC\Memcache\Redis; + /** * @group Memcache * @group Redis */ class RedisTest extends Cache { + /** + * @var Redis cache; + */ + protected $instance; + public static function setUpBeforeClass(): void { parent::setUpBeforeClass(); @@ -62,4 +69,18 @@ public function testScriptHashes() { $this->assertEquals(sha1($script[0]), $script[1]); } } + + public function testCasTtlNotChanged() { + $this->instance->set('foo', 'bar', 50); + $this->assertTrue($this->instance->compareSetTTL('foo', 'bar', 100)); + // allow for 1s of inaccuracy due to time moving forward + $this->assertLessThan(1, 100 - $this->instance->getTTL('foo')); + } + + public function testCasTtlChanged() { + $this->instance->set('foo', 'bar1', 50); + $this->assertFalse($this->instance->compareSetTTL('foo', 'bar', 100)); + // allow for 1s of inaccuracy due to time moving forward + $this->assertLessThan(1, 50 - $this->instance->getTTL('foo')); + } }