Merge pull request #19098 from owncloud/db-lock-no-transaction

Remove the need for the transaction in the database locking backend

lib/private/lock/dblockingprovider.php

@@ -21,6 +21,7 @@
 
 namespace OC\Lock;
 
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IDBConnection;
 use OCP\ILogger;
 use OCP\Lock\LockedException;
@@ -39,17 +40,34 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 */
 	private $logger;
 
+	/**
+	 * @var \OCP\AppFramework\Utility\ITimeFactory
+	 */
+	private $timeFactory;
+
+	const TTL = 3600; // how long until we clear stray locks in seconds
+
 	/**
 	 * @param \OCP\IDBConnection $connection
 	 * @param \OCP\ILogger $logger
+	 * @param \OCP\AppFramework\Utility\ITimeFactory $timeFactory
 	 */
-	public function __construct(IDBConnection $connection, ILogger $logger) {
+	public function __construct(IDBConnection $connection, ILogger $logger, ITimeFactory $timeFactory) {
 		$this->connection = $connection;
 		$this->logger = $logger;
+		$this->timeFactory = $timeFactory;
 	}
 
 	protected function initLockField($path) {
-		$this->connection->insertIfNotExist('*PREFIX*file_locks', ['key' => $path, 'lock' => 0, 'ttl' => 0], ['key']);
+		$expire = $this->getExpireTime();
+		$this->connection->insertIfNotExist('*PREFIX*file_locks', ['key' => $path, 'lock' => 0, 'ttl' => $expire], ['key']);
+	}
+
+	/**
+	 * @return int
+	 */
+	protected function getExpireTime() {
+		return $this->timeFactory->getTime() + self::TTL;
 	}
 
 	/**
@@ -76,24 +94,23 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 * @throws \OCP\Lock\LockedException
 	 */
 	public function acquireLock($path, $type) {
-		if ($this->connection->inTransaction()){
+		if ($this->connection->inTransaction()) {
 			$this->logger->warning("Trying to acquire a lock for '$path' while inside a transition");
 		}
 
-		$this->connection->beginTransaction();
 		$this->initLockField($path);
+		$expire = $this->getExpireTime();
 		if ($type === self::LOCK_SHARED) {
 			$result = $this->connection->executeUpdate(
-				'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1 WHERE `key` = ? AND `lock` >= 0',
+				'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1, `ttl` = ? WHERE `key` = ? AND `lock` >= 0',
-				[$path]
+				[$expire, $path]
 			);
 		} else {
 			$result = $this->connection->executeUpdate(
-				'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `key` = ? AND `lock` = 0',
+				'UPDATE `*PREFIX*file_locks` SET `lock` = -1, `ttl` = ? WHERE `key` = ? AND `lock` = 0',
-				[$path]
+				[$expire, $path]
 			);
 		}
-		$this->connection->commit();
 		if ($result !== 1) {
 			throw new LockedException($path);
 		}
@@ -129,16 +146,16 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 * @throws \OCP\Lock\LockedException
 	 */
 	public function changeLock($path, $targetType) {
-		$this->initLockField($path);
+		$expire = $this->getExpireTime();
 		if ($targetType === self::LOCK_SHARED) {
 			$result = $this->connection->executeUpdate(
-				'UPDATE `*PREFIX*file_locks` SET `lock` = 1 WHERE `key` = ? AND `lock` = -1',
+				'UPDATE `*PREFIX*file_locks` SET `lock` = 1, `ttl` = ? WHERE `key` = ? AND `lock` = -1',
-				[$path]
+				[$expire, $path]
 			);
 		} else {
 			$result = $this->connection->executeUpdate(
-				'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `key` = ? AND `lock` = 1',
+				'UPDATE `*PREFIX*file_locks` SET `lock` = -1, `ttl` = ? WHERE `key` = ? AND `lock` = 1',
-				[$path]
+				[$expire, $path]
 			);
 		}
 		if ($result !== 1) {
@@ -151,8 +168,10 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 * cleanup empty locks
 	 */
 	public function cleanEmptyLocks() {
+		$expire = $this->timeFactory->getTime();
 		$this->connection->executeUpdate(
-			'DELETE FROM `*PREFIX*file_locks` WHERE `lock` = 0'
+			'DELETE FROM `*PREFIX*file_locks` WHERE `lock` = 0 AND `ttl` < ?',
+			[$expire]
 		);
 	}
 

lib/private/server.php

@@ -460,7 +460,7 @@ class Server extends SimpleContainer implements IServerContainer {
 				if (!($memcache instanceof \OC\Memcache\NullCache)) {
 					return new MemcacheLockingProvider($memcache);
 				}
-				return new DBLockingProvider($c->getDatabaseConnection(), $c->getLogger());
+				return new DBLockingProvider($c->getDatabaseConnection(), $c->getLogger(), new TimeFactory());
 			}
 			return new NoopLockingProvider();
 		});

tests/lib/lock/dblockingprovider.php

@@ -21,23 +21,77 @@
 
 namespace Test\Lock;
 
+use OCP\Lock\ILockingProvider;
+
 class DBLockingProvider extends LockingProvider {
+	/**
+	 * @var \OC\Lock\DBLockingProvider
+	 */
+	protected $instance;
 
 	/**
 	 * @var \OCP\IDBConnection
 	 */
 	private $connection;
 
+	/**
+	 * @var \OCP\AppFramework\Utility\ITimeFactory
+	 */
+	private $timeFactory;
+
+	private $currentTime;
+
+	public function setUp() {
+		$this->currentTime = time();
+		$this->timeFactory = $this->getMock('\OCP\AppFramework\Utility\ITimeFactory');
+		$this->timeFactory->expects($this->any())
+			->method('getTime')
+			->will($this->returnCallback(function () {
+				return $this->currentTime;
+			}));
+		parent::setUp();
+	}
+
 	/**
 	 * @return \OCP\Lock\ILockingProvider
 	 */
 	protected function getInstance() {
 		$this->connection = \OC::$server->getDatabaseConnection();
-		return new \OC\Lock\DBLockingProvider($this->connection, \OC::$server->getLogger());
+		return new \OC\Lock\DBLockingProvider($this->connection, \OC::$server->getLogger(), $this->timeFactory);
 	}
 
 	public function tearDown() {
 		$this->connection->executeQuery('DELETE FROM `*PREFIX*file_locks`');
 		parent::tearDown();
 	}
+
+	public function testCleanEmptyLocks() {
+		$this->currentTime = 100;
+		$this->instance->acquireLock('foo', ILockingProvider::LOCK_EXCLUSIVE);
+		$this->instance->acquireLock('asd', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$this->currentTime = 200;
+		$this->instance->acquireLock('bar', ILockingProvider::LOCK_EXCLUSIVE);
+		$this->instance->changeLock('asd', ILockingProvider::LOCK_SHARED);
+
+		$this->currentTime = 150 + \OC\Lock\DBLockingProvider::TTL;
+
+		$this->assertEquals(3, $this->getLockEntryCount());
+
+		$this->instance->cleanEmptyLocks();
+
+		$this->assertEquals(3, $this->getLockEntryCount());
+
+		$this->instance->releaseAll();
+
+		$this->instance->cleanEmptyLocks();
+
+		$this->assertEquals(2, $this->getLockEntryCount());
+	}
+
+	private function getLockEntryCount() {
+		$query = $this->connection->prepare('SELECT count(*) FROM `*PREFIX*file_locks`');
+		$query->execute();
+		return $query->fetchColumn();
+	}
 }