Merge pull request #19890 from owncloud/db-keep-shared-locks

Keep shared locks until the end of the request so we can reuse them

2 files changed, 107 insertions, 18 deletions

diff --git a/lib/private/lock/abstractlockingprovider.php b/lib/private/lock/abstractlockingprovider.php
index eb86be68500..c7a29380efe 100644
--- a/lib/private/lock/abstractlockingprovider.php
+++ b/lib/private/lock/abstractlockingprovider.php
@@ -34,6 +34,21 @@ abstract class AbstractLockingProvider implements ILockingProvider {
 	];
 
 	/**
+	 * Check if we've locally acquired a lock
+	 *
+	 * @param string $path
+	 * @param int $type
+	 * @return bool
+	 */
+	protected function hasAcquiredLock($path, $type) {
+		if ($type === self::LOCK_SHARED) {
+			return isset($this->acquiredLocks['shared'][$path]) && $this->acquiredLocks['shared'][$path] > 0;
+		} else {
+			return isset($this->acquiredLocks['exclusive'][$path]) && $this->acquiredLocks['exclusive'][$path] === true;
+		}
+	}
+
+	/**
 	 * Mark a locally acquired lock
 	 *
 	 * @param string $path
diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php
index 450a0a27ab0..a167687e0ac 100644
--- a/lib/private/lock/dblockingprovider.php
+++ b/lib/private/lock/dblockingprovider.php
@@ -25,6 +25,7 @@ namespace OC\Lock;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IDBConnection;
 use OCP\ILogger;
+use OCP\Lock\ILockingProvider;
 use OCP\Lock\LockedException;
 
 /**
@@ -46,9 +47,49 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 */
 	private $timeFactory;
 
+	private $sharedLocks = [];
+
 	const TTL = 3600; // how long until we clear stray locks in seconds
 
 	/**
+	 * Check if we have an open shared lock for a path
+	 *
+	 * @param string $path
+	 * @return bool
+	 */
+	protected function isLocallyLocked($path) {
+		return isset($this->sharedLocks[$path]) && $this->sharedLocks[$path];
+	}
+
+	/**
+	 * Mark a locally acquired lock
+	 *
+	 * @param string $path
+	 * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE
+	 */
+	protected function markAcquire($path, $type) {
+		parent::markAcquire($path, $type);
+		if ($type === self::LOCK_SHARED) {
+			$this->sharedLocks[$path] = true;
+		}
+	}
+
+	/**
+	 * Change the type of an existing tracked lock
+	 *
+	 * @param string $path
+	 * @param int $targetType self::LOCK_SHARED or self::LOCK_EXCLUSIVE
+	 */
+	protected function markChange($path, $targetType) {
+		parent::markChange($path, $targetType);
+		if ($targetType === self::LOCK_SHARED) {
+			$this->sharedLocks[$path] = true;
+		} else if ($targetType === self::LOCK_EXCLUSIVE) {
+			$this->sharedLocks[$path] = false;
+		}
+	}
+
+	/**
 	 * @param \OCP\IDBConnection $connection
 	 * @param \OCP\ILogger $logger
 	 * @param \OCP\AppFramework\Utility\ITimeFactory $timeFactory
@@ -85,11 +126,19 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 * @return bool
 	 */
 	public function isLocked($path, $type) {
+		if ($this->hasAcquiredLock($path, $type)) {
+			return true;
+		}
 		$query = $this->connection->prepare('SELECT `lock` from `*PREFIX*file_locks` WHERE `key` = ?');
 		$query->execute([$path]);
 		$lockValue = (int)$query->fetchColumn();
 		if ($type === self::LOCK_SHARED) {
-			return $lockValue > 0;
+			if ($this->isLocallyLocked($path)) {
+				// if we have a shared lock we kept open locally but it's released we always have at least 1 shared lock in the db
+				return $lockValue > 1;
+			} else {
+				return $lockValue > 0;
+			}
 		} else if ($type === self::LOCK_EXCLUSIVE) {
 			return $lockValue === -1;
 		} else {
@@ -105,19 +154,27 @@ class DBLockingProvider extends AbstractLockingProvider {
 	public function acquireLock($path, $type) {
 		$expire = $this->getExpireTime();
 		if ($type === self::LOCK_SHARED) {
-			$result = $this->initLockField($path,1);
-			if ($result <= 0) {
-				$result = $this->connection->executeUpdate (
-					'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1, `ttl` = ? WHERE `key` = ? AND `lock` >= 0',
-					[$expire, $path]
-				);
+			if (!$this->isLocallyLocked($path)) {
+				$result = $this->initLockField($path, 1);
+				if ($result <= 0) {
+					$result = $this->connection->executeUpdate(
+						'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1, `ttl` = ? WHERE `key` = ? AND `lock` >= 0',
+						[$expire, $path]
+					);
+				}
+			} else {
+				$result = 1;
 			}
 		} else {
-			$result = $this->initLockField($path,-1);
+			$existing = 0;
+			if ($this->hasAcquiredLock($path, ILockingProvider::LOCK_SHARED) === false && $this->isLocallyLocked($path)) {
+				$existing = 1;
+			}
+			$result = $this->initLockField($path, -1);
 			if ($result <= 0) {
 				$result = $this->connection->executeUpdate(
-					'UPDATE `*PREFIX*file_locks` SET `lock` = -1, `ttl` = ? WHERE `key` = ? AND `lock` = 0',
-					[$expire, $path]
+					'UPDATE `*PREFIX*file_locks` SET `lock` = -1, `ttl` = ? WHERE `key` = ? AND `lock` = ?',
+					[$expire, $path, $existing]
 				);
 			}
 		}
@@ -132,19 +189,15 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE
 	 */
 	public function releaseLock($path, $type) {
-		if ($type === self::LOCK_SHARED) {
-			$this->connection->executeUpdate(
-				'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` - 1 WHERE `key` = ? AND `lock` > 0',
-				[$path]
-			);
-		} else {
+		$this->markRelease($path, $type);
+
+		// we keep shared locks till the end of the request so we can re-use them
+		if ($type === self::LOCK_EXCLUSIVE) {
 			$this->connection->executeUpdate(
 				'UPDATE `*PREFIX*file_locks` SET `lock` = 0 WHERE `key` = ? AND `lock` = -1',
 				[$path]
 			);
 		}
-
-		$this->markRelease($path, $type);
 	}
 
 	/**
@@ -162,6 +215,10 @@ class DBLockingProvider extends AbstractLockingProvider {
 				[$expire, $path]
 			);
 		} else {
+			// since we only keep one shared lock in the db we need to check if we have more then one shared lock locally manually
+			if (isset($this->acquiredLocks['shared'][$path]) && $this->acquiredLocks['shared'][$path] > 1) {
+				throw new LockedException($path);
+			}
 			$result = $this->connection->executeUpdate(
 				'UPDATE `*PREFIX*file_locks` SET `lock` = -1, `ttl` = ? WHERE `key` = ? AND `lock` = 1',
 				[$expire, $path]
@@ -184,6 +241,23 @@ class DBLockingProvider extends AbstractLockingProvider {
 		);
 	}
 
+	/**
+	 * release all lock acquired by this instance which were marked using the mark* methods
+	 */
+	public function releaseAll() {
+		parent::releaseAll();
+
+		// since we keep shared locks we need to manually clean those
+		foreach ($this->sharedLocks as $path => $lock) {
+			if ($lock) {
+				$this->connection->executeUpdate(
+					'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` - 1 WHERE `key` = ? AND `lock` > 0',
+					[$path]
+				);
+			}
+		}
+	}
+
 	public function __destruct() {
 		try {
 			$this->cleanEmptyLocks();