diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index f3e684d0b4..bfa86a6920 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/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', - [$path] + 'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1, `ttl` = ? WHERE `key` = ? AND `lock` >= 0', + [$expire, $path] ); } else { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `key` = ? AND `lock` = 0', - [$path] + 'UPDATE `*PREFIX*file_locks` SET `lock` = -1, `ttl` = ? WHERE `key` = ? AND `lock` = 0', + [$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', - [$path] + 'UPDATE `*PREFIX*file_locks` SET `lock` = 1, `ttl` = ? WHERE `key` = ? AND `lock` = -1', + [$expire, $path] ); } else { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `key` = ? AND `lock` = 1', - [$path] + 'UPDATE `*PREFIX*file_locks` SET `lock` = -1, `ttl` = ? WHERE `key` = ? AND `lock` = 1', + [$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] ); } diff --git a/lib/private/server.php b/lib/private/server.php index 9657afbdae..d5f4f532c1 100644 --- a/lib/private/server.php +++ b/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(); }); diff --git a/tests/lib/lock/dblockingprovider.php b/tests/lib/lock/dblockingprovider.php index fd6550d9c4..2360052b4a 100644 --- a/tests/lib/lock/dblockingprovider.php +++ b/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(); + } }