Merge pull request #37469 from nextcloud/lock-restore-ttl

restore shared lock ttl to previous value when releasing

Author: sorbaugh

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<string, array{time: int, ttl: int}> */
+	private array $oldTTLs = [];
+
 	public function __construct(
 		private IMemcache $memcache,
+		private ITimeFactory $timeFactory,
 		int $ttl = 3600,
 	) {
 		parent::__construct($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) {

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

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

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',
+		],
 	];
 
 	/**
@@ -181,6 +185,17 @@ 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 {
+		$value = self::encodeValue($value);
+
+		return $this->evalLua('caSetTtl', [$key], [$value, $ttl]) > 0;
+	}
+
 	public static function isAvailable(): bool {
 		return \OC::$server->getGetRedisFactory()->isAvailable();
 	}

lib/private/Server.php

@@ -173,6 +173,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;
@@ -1079,7 +1080,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),

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

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 {

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