From 7c66328381dd0e2ce5e7ab1e4477593bc79ed3ad Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 16 Sep 2015 17:29:34 +0200 Subject: [PATCH 1/3] Remove the need for the transaction in the database locking backend --- lib/private/lock/dblockingprovider.php | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index f3e684d0b4..6f14b7806e 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -76,11 +76,10 @@ 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); if ($type === self::LOCK_SHARED) { $result = $this->connection->executeUpdate( @@ -93,7 +92,6 @@ class DBLockingProvider extends AbstractLockingProvider { [$path] ); } - $this->connection->commit(); if ($result !== 1) { throw new LockedException($path); } @@ -146,17 +144,4 @@ class DBLockingProvider extends AbstractLockingProvider { } $this->markChange($path, $targetType); } - - /** - * cleanup empty locks - */ - public function cleanEmptyLocks() { - $this->connection->executeUpdate( - 'DELETE FROM `*PREFIX*file_locks` WHERE `lock` = 0' - ); - } - - public function __destruct() { - $this->cleanEmptyLocks(); - } } From 05fddec022488493c3f346797684231519f6746c Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 17 Sep 2015 13:55:04 +0200 Subject: [PATCH 2/3] expire old lock rows --- lib/private/lock/dblockingprovider.php | 56 +++++++++++++++++++++----- lib/private/server.php | 2 +- tests/lib/lock/dblockingprovider.php | 56 +++++++++++++++++++++++++- 3 files changed, 101 insertions(+), 13 deletions(-) diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index 6f14b7806e..ce2960bbf6 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 + /** * @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; } /** @@ -81,15 +99,16 @@ class DBLockingProvider extends AbstractLockingProvider { } $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] ); } if ($result !== 1) { @@ -127,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) { @@ -144,4 +163,19 @@ class DBLockingProvider extends AbstractLockingProvider { } $this->markChange($path, $targetType); } + + /** + * cleanup empty locks + */ + public function cleanEmptyLocks() { + $expire = $this->timeFactory->getTime(); + $this->connection->executeUpdate( + 'DELETE FROM `*PREFIX*file_locks` WHERE `lock` = 0 AND `ttl` < ?', + [$expire] + ); + } + + public function __destruct() { + $this->cleanEmptyLocks(); + } } 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(); + } } From e9b1aa603715fcbefdef8cb96b042fa2f0348bf1 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 17 Sep 2015 14:09:28 +0200 Subject: [PATCH 3/3] improve doc --- lib/private/lock/dblockingprovider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index ce2960bbf6..bfa86a6920 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -45,7 +45,7 @@ class DBLockingProvider extends AbstractLockingProvider { */ private $timeFactory; - const TTL = 3600; // how long until we clear stray locks + const TTL = 3600; // how long until we clear stray locks in seconds /** * @param \OCP\IDBConnection $connection