From a8552a1b24e7df8c4822b5b0dd7c690312ae810d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 15 Jul 2015 15:44:51 +0200 Subject: [PATCH 01/12] split off keeping track of acquire locks --- lib/private/lock/abstractlockingprovider.php | 96 ++++++++++++++++++++ lib/private/lock/memcachelockingprovider.php | 40 +------- tests/lib/lock/lockingprovider.php | 30 ++++++ 3 files changed, 130 insertions(+), 36 deletions(-) create mode 100644 lib/private/lock/abstractlockingprovider.php diff --git a/lib/private/lock/abstractlockingprovider.php b/lib/private/lock/abstractlockingprovider.php new file mode 100644 index 00000000000..290bb77e52f --- /dev/null +++ b/lib/private/lock/abstractlockingprovider.php @@ -0,0 +1,96 @@ + + * @author Robin Appelman + * @author Thomas Müller + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Lock; + +use OCP\Lock\ILockingProvider; + +abstract class AbstractLockingProvider implements ILockingProvider { + protected $acquiredLocks = [ + 'shared' => [], + 'exclusive' => [] + ]; + + /** + * @param string $path + * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE + */ + protected function markAcquire($path, $type) { + if ($type === self::LOCK_SHARED) { + if (!isset($this->acquiredLocks['shared'][$path])) { + $this->acquiredLocks['shared'][$path] = 0; + } + $this->acquiredLocks['shared'][$path]++; + } else { + $this->acquiredLocks['exclusive'][$path] = true; + } + } + + /** + * @param string $path + * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE + */ + protected function markRelease($path, $type) { + if ($type === self::LOCK_SHARED) { + if (isset($this->acquiredLocks['shared'][$path]) and $this->acquiredLocks['shared'][$path] > 0) { + $this->acquiredLocks['shared'][$path]--; + } + } else if ($type === self::LOCK_EXCLUSIVE) { + unset($this->acquiredLocks['exclusive'][$path]); + } + } + + /** + * Change the type of an existing lock + * + * @param string $path + * @param int $targetType self::LOCK_SHARED or self::LOCK_EXCLUSIVE + */ + protected function markChange($path, $targetType) { + if ($targetType === self::LOCK_SHARED) { + unset($this->acquiredLocks['exclusive'][$path]); + if (!isset($this->acquiredLocks['shared'][$path])) { + $this->acquiredLocks['shared'][$path] = 0; + } + $this->acquiredLocks['shared'][$path]++; + } else if ($targetType === self::LOCK_EXCLUSIVE) { + $this->acquiredLocks['exclusive'][$path] = true; + $this->acquiredLocks['shared'][$path]--; + } + } + + /** + * release all lock acquired by this instance + */ + public function releaseAll() { + foreach ($this->acquiredLocks['shared'] as $path => $count) { + for ($i = 0; $i < $count; $i++) { + $this->releaseLock($path, self::LOCK_SHARED); + } + } + + foreach ($this->acquiredLocks['exclusive'] as $path => $hasLock) { + $this->releaseLock($path, self::LOCK_EXCLUSIVE); + } + } +} diff --git a/lib/private/lock/memcachelockingprovider.php b/lib/private/lock/memcachelockingprovider.php index 5f2b5e5a4b8..871572f7e3e 100644 --- a/lib/private/lock/memcachelockingprovider.php +++ b/lib/private/lock/memcachelockingprovider.php @@ -23,21 +23,15 @@ namespace OC\Lock; -use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use OCP\IMemcache; -class MemcacheLockingProvider implements ILockingProvider { +class MemcacheLockingProvider extends AbstractLockingProvider { /** * @var \OCP\IMemcache */ private $memcache; - private $acquiredLocks = [ - 'shared' => [], - 'exclusive' => [] - ]; - /** * @param \OCP\IMemcache $memcache */ @@ -71,17 +65,13 @@ class MemcacheLockingProvider implements ILockingProvider { if (!$this->memcache->inc($path)) { throw new LockedException($path); } - if (!isset($this->acquiredLocks['shared'][$path])) { - $this->acquiredLocks['shared'][$path] = 0; - } - $this->acquiredLocks['shared'][$path]++; } else { $this->memcache->add($path, 0); if (!$this->memcache->cas($path, 0, 'exclusive')) { throw new LockedException($path); } - $this->acquiredLocks['exclusive'][$path] = true; } + $this->markAcquire($path, $type); } /** @@ -92,13 +82,12 @@ class MemcacheLockingProvider implements ILockingProvider { if ($type === self::LOCK_SHARED) { if (isset($this->acquiredLocks['shared'][$path]) and $this->acquiredLocks['shared'][$path] > 0) { $this->memcache->dec($path); - $this->acquiredLocks['shared'][$path]--; $this->memcache->cad($path, 0); } } else if ($type === self::LOCK_EXCLUSIVE) { $this->memcache->cad($path, 'exclusive'); - unset($this->acquiredLocks['exclusive'][$path]); } + $this->markRelease($path, $type); } /** @@ -113,33 +102,12 @@ class MemcacheLockingProvider implements ILockingProvider { if (!$this->memcache->cas($path, 'exclusive', 1)) { throw new LockedException($path); } - unset($this->acquiredLocks['exclusive'][$path]); - if (!isset($this->acquiredLocks['shared'][$path])) { - $this->acquiredLocks['shared'][$path] = 0; - } - $this->acquiredLocks['shared'][$path]++; } else if ($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')) { throw new LockedException($path); } - $this->acquiredLocks['exclusive'][$path] = true; - $this->acquiredLocks['shared'][$path]--; - } - } - - /** - * release all lock acquired by this instance - */ - public function releaseAll() { - foreach ($this->acquiredLocks['shared'] as $path => $count) { - for ($i = 0; $i < $count; $i++) { - $this->releaseLock($path, self::LOCK_SHARED); - } - } - - foreach ($this->acquiredLocks['exclusive'] as $path => $hasLock) { - $this->releaseLock($path, self::LOCK_EXCLUSIVE); } + $this->markChange($path, $targetType); } } diff --git a/tests/lib/lock/lockingprovider.php b/tests/lib/lock/lockingprovider.php index efd6e1939f2..ca72c1bb7f3 100644 --- a/tests/lib/lock/lockingprovider.php +++ b/tests/lib/lock/lockingprovider.php @@ -120,6 +120,36 @@ abstract class LockingProvider extends TestCase { $this->assertFalse($this->instance->isLocked('asd', ILockingProvider::LOCK_EXCLUSIVE)); } + public function testReleaseAllAfterChange() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('bar', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('asd', ILockingProvider::LOCK_EXCLUSIVE); + + $this->instance->changeLock('bar', ILockingProvider::LOCK_EXCLUSIVE); + + $this->instance->releaseAll(); + + $this->assertFalse($this->instance->isLocked('foo', ILockingProvider::LOCK_SHARED)); + $this->assertFalse($this->instance->isLocked('bar', ILockingProvider::LOCK_SHARED)); + $this->assertFalse($this->instance->isLocked('bar', ILockingProvider::LOCK_EXCLUSIVE)); + $this->assertFalse($this->instance->isLocked('asd', ILockingProvider::LOCK_EXCLUSIVE)); + } + + public function testReleaseAllAfterUnlock() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('bar', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('asd', ILockingProvider::LOCK_EXCLUSIVE); + + $this->instance->releaseLock('bar', ILockingProvider::LOCK_SHARED); + + $this->instance->releaseAll(); + + $this->assertFalse($this->instance->isLocked('foo', ILockingProvider::LOCK_SHARED)); + $this->assertFalse($this->instance->isLocked('asd', ILockingProvider::LOCK_EXCLUSIVE)); + } + public function testReleaseAfterReleaseAll() { $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); From 4ea7cbb0f5d97b0ae5fe8a6c3c43718d3fa5172e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 15 Jul 2015 16:14:32 +0200 Subject: [PATCH 02/12] Add database backend for high level locking --- db_structure.xml | 39 ++++++++ lib/private/lock/dblockingprovider.php | 131 +++++++++++++++++++++++++ lib/repair/dropoldtables.php | 1 - tests/lib/lock/dblockingprovider.php | 43 ++++++++ version.php | 2 +- 5 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 lib/private/lock/dblockingprovider.php create mode 100644 tests/lib/lock/dblockingprovider.php diff --git a/db_structure.xml b/db_structure.xml index 6d1cf6973c5..95acefcfaee 100644 --- a/db_structure.xml +++ b/db_structure.xml @@ -1180,5 +1180,44 @@ + + + + *dbprefix*locks + + + + + lock + integer + 0 + true + 4 + + + + path + text + true + 64 + + + + + true + true + lock_path_index + + path + ascending + + + + + +
+ diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php new file mode 100644 index 00000000000..70f4539eb28 --- /dev/null +++ b/lib/private/lock/dblockingprovider.php @@ -0,0 +1,131 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Lock; + +use OCP\IDBConnection; +use OCP\Lock\LockedException; + +class DBLockingProvider extends AbstractLockingProvider { + /** + * @var \OCP\IDBConnection + */ + private $connection; + + /** + * @param \OCP\IDBConnection $connection + */ + public function __construct(IDBConnection $connection) { + $this->connection = $connection; + } + + protected function initLockField($path) { + $this->connection->insertIfNotExist('*PREFIX*locks', ['path' => $path, 'lock' => 0], ['path']); + } + + /** + * @param string $path + * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE + * @return bool + */ + public function isLocked($path, $type) { + $query = $this->connection->prepare('SELECT `lock` from `*PREFIX*locks` WHERE `path` = ?'); + $query->execute([$path]); + $lockValue = (int)$query->fetchColumn(); + if ($type === self::LOCK_SHARED) { + return $lockValue > 0; + } else if ($type === self::LOCK_EXCLUSIVE) { + return $lockValue === -1; + } else { + return false; + } + } + + /** + * @param string $path + * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE + * @throws \OCP\Lock\LockedException + */ + public function acquireLock($path, $type) { + $this->initLockField($path); + if ($type === self::LOCK_SHARED) { + $result = $this->connection->executeUpdate( + 'UPDATE `*PREFIX*locks` SET `lock` = `lock` + 1 WHERE `path` = ? AND `lock` >= 0', + [$path] + ); + } else { + $result = $this->connection->executeUpdate( + 'UPDATE `*PREFIX*locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 0', + [$path] + ); + } + if ($result !== 1) { + throw new LockedException($path); + } + $this->markAcquire($path, $type); + } + + /** + * @param string $path + * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE + */ + public function releaseLock($path, $type) { + $this->initLockField($path); + if ($type === self::LOCK_SHARED) { + $this->connection->executeUpdate( + 'UPDATE `*PREFIX*locks` SET `lock` = `lock` - 1 WHERE `path` = ? AND `lock` > 0', + [$path] + ); + } else { + $this->connection->executeUpdate( + 'UPDATE `*PREFIX*locks` SET `lock` = 0 WHERE `path` = ? AND `lock` = -1', + [$path] + ); + } + $this->markRelease($path, $type); + } + + /** + * Change the type of an existing lock + * + * @param string $path + * @param int $targetType self::LOCK_SHARED or self::LOCK_EXCLUSIVE + * @throws \OCP\Lock\LockedException + */ + public function changeLock($path, $targetType) { + $this->initLockField($path); + if ($targetType === self::LOCK_SHARED) { + $result = $this->connection->executeUpdate( + 'UPDATE `*PREFIX*locks` SET `lock` = 1 WHERE `path` = ? AND `lock` = -1', + [$path] + ); + } else { + $result = $this->connection->executeUpdate( + 'UPDATE `*PREFIX*locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 1', + [$path] + ); + } + if ($result !== 1) { + throw new LockedException($path); + } + $this->markChange($path, $targetType); + } +} diff --git a/lib/repair/dropoldtables.php b/lib/repair/dropoldtables.php index cfe0df6cb5b..89f872e16cc 100644 --- a/lib/repair/dropoldtables.php +++ b/lib/repair/dropoldtables.php @@ -76,7 +76,6 @@ class DropOldTables extends BasicEmitter implements RepairStep { 'calendar_share_event', 'foldersize', 'fscache', - 'locks', 'log', 'media_albums', 'media_artists', diff --git a/tests/lib/lock/dblockingprovider.php b/tests/lib/lock/dblockingprovider.php new file mode 100644 index 00000000000..4229ffb9c51 --- /dev/null +++ b/tests/lib/lock/dblockingprovider.php @@ -0,0 +1,43 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace Test\Lock; + +class DBLockingProvider extends LockingProvider { + + /** + * @var \OCP\IDBConnection + */ + private $connection; + + /** + * @return \OCP\Lock\ILockingProvider + */ + protected function getInstance() { + $this->connection = \OC::$server->getDatabaseConnection(); + return new \OC\Lock\DBLockingProvider($this->connection); + } + + public function tearDown() { + $this->connection->executeQuery('DELETE FROM `*PREFIX*locks`'); + parent::tearDown(); + } +} diff --git a/version.php b/version.php index 7ccd2e6b548..a115f4b26be 100644 --- a/version.php +++ b/version.php @@ -22,7 +22,7 @@ // We only can count up. The 4. digit is only for the internal patchlevel to trigger DB upgrades // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel // when updating major/minor version number. -$OC_Version=array(8, 2, 0, 3); +$OC_Version=array(8, 2, 0, 4); // The human readable string $OC_VersionString='8.2 pre alpha'; From 86acd535c243569fda7c1a2957073509c7e94f89 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 15 Jul 2015 16:14:48 +0200 Subject: [PATCH 03/12] use the database backend for locking if no memcache is configured for it --- lib/private/server.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/private/server.php b/lib/private/server.php index 12981fe7f19..51e70405432 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -48,6 +48,7 @@ use OC\Diagnostics\QueryLogger; use OC\Files\Node\Root; use OC\Files\View; use OC\Http\Client\ClientService; +use OC\Lock\DBLockingProvider; use OC\Lock\MemcacheLockingProvider; use OC\Lock\NoopLockingProvider; use OC\Mail\Mailer; @@ -434,10 +435,7 @@ class Server extends SimpleContainer implements IServerContainer { if (!($memcache instanceof \OC\Memcache\NullCache)) { return new MemcacheLockingProvider($memcache); } - throw new HintException( - 'File locking is enabled but the locking cache class was not found', - 'Please check the "memcache.locking" setting and make sure the matching PHP module is installed and enabled' - ); + return new DBLockingProvider($c->getDatabaseConnection()); } return new NoopLockingProvider(); }); From 96a9d171b3fc31a6ad1ed89dca987765ed1ce721 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 16 Jul 2015 12:55:28 +0200 Subject: [PATCH 04/12] Fix db schema --- db_structure.xml | 35 +++++++++++++++++++- lib/private/lock/abstractlockingprovider.php | 2 -- lib/private/lock/dblockingprovider.php | 16 ++++----- lib/repair/dropoldtables.php | 1 + tests/lib/lock/dblockingprovider.php | 2 +- 5 files changed, 44 insertions(+), 12 deletions(-) diff --git a/db_structure.xml b/db_structure.xml index 95acefcfaee..5c2b26e5f15 100644 --- a/db_structure.xml +++ b/db_structure.xml @@ -1185,10 +1185,20 @@ - *dbprefix*locks + *dbprefix*file_locks + + id + integer + 0 + true + true + 4 + 1 + + lock integer @@ -1204,9 +1214,24 @@ 64 + + ttl + integer + true + 4 + true + true + lock_id_index + + id + ascending + + + + true lock_path_index @@ -1215,6 +1240,14 @@ + + lock_ttl_index + + ttl + ascending + + + diff --git a/lib/private/lock/abstractlockingprovider.php b/lib/private/lock/abstractlockingprovider.php index 290bb77e52f..03b4912ea13 100644 --- a/lib/private/lock/abstractlockingprovider.php +++ b/lib/private/lock/abstractlockingprovider.php @@ -1,8 +1,6 @@ * @author Robin Appelman - * @author Thomas Müller * * @copyright Copyright (c) 2015, ownCloud, Inc. * @license AGPL-3.0 diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index 70f4539eb28..aee66bca745 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -38,7 +38,7 @@ class DBLockingProvider extends AbstractLockingProvider { } protected function initLockField($path) { - $this->connection->insertIfNotExist('*PREFIX*locks', ['path' => $path, 'lock' => 0], ['path']); + $this->connection->insertIfNotExist('*PREFIX*file_locks', ['path' => $path, 'lock' => 0], ['path']); } /** @@ -47,7 +47,7 @@ class DBLockingProvider extends AbstractLockingProvider { * @return bool */ public function isLocked($path, $type) { - $query = $this->connection->prepare('SELECT `lock` from `*PREFIX*locks` WHERE `path` = ?'); + $query = $this->connection->prepare('SELECT `lock` from `*PREFIX*file_locks` WHERE `path` = ?'); $query->execute([$path]); $lockValue = (int)$query->fetchColumn(); if ($type === self::LOCK_SHARED) { @@ -68,12 +68,12 @@ class DBLockingProvider extends AbstractLockingProvider { $this->initLockField($path); if ($type === self::LOCK_SHARED) { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*locks` SET `lock` = `lock` + 1 WHERE `path` = ? AND `lock` >= 0', + 'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1 WHERE `path` = ? AND `lock` >= 0', [$path] ); } else { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 0', + 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 0', [$path] ); } @@ -91,12 +91,12 @@ class DBLockingProvider extends AbstractLockingProvider { $this->initLockField($path); if ($type === self::LOCK_SHARED) { $this->connection->executeUpdate( - 'UPDATE `*PREFIX*locks` SET `lock` = `lock` - 1 WHERE `path` = ? AND `lock` > 0', + 'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` - 1 WHERE `path` = ? AND `lock` > 0', [$path] ); } else { $this->connection->executeUpdate( - 'UPDATE `*PREFIX*locks` SET `lock` = 0 WHERE `path` = ? AND `lock` = -1', + 'UPDATE `*PREFIX*file_locks` SET `lock` = 0 WHERE `path` = ? AND `lock` = -1', [$path] ); } @@ -114,12 +114,12 @@ class DBLockingProvider extends AbstractLockingProvider { $this->initLockField($path); if ($targetType === self::LOCK_SHARED) { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*locks` SET `lock` = 1 WHERE `path` = ? AND `lock` = -1', + 'UPDATE `*PREFIX*file_locks` SET `lock` = 1 WHERE `path` = ? AND `lock` = -1', [$path] ); } else { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 1', + 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 1', [$path] ); } diff --git a/lib/repair/dropoldtables.php b/lib/repair/dropoldtables.php index 89f872e16cc..cfe0df6cb5b 100644 --- a/lib/repair/dropoldtables.php +++ b/lib/repair/dropoldtables.php @@ -76,6 +76,7 @@ class DropOldTables extends BasicEmitter implements RepairStep { 'calendar_share_event', 'foldersize', 'fscache', + 'locks', 'log', 'media_albums', 'media_artists', diff --git a/tests/lib/lock/dblockingprovider.php b/tests/lib/lock/dblockingprovider.php index 4229ffb9c51..4818ef1ceca 100644 --- a/tests/lib/lock/dblockingprovider.php +++ b/tests/lib/lock/dblockingprovider.php @@ -37,7 +37,7 @@ class DBLockingProvider extends LockingProvider { } public function tearDown() { - $this->connection->executeQuery('DELETE FROM `*PREFIX*locks`'); + $this->connection->executeQuery('DELETE FROM `*PREFIX*file_locks`'); parent::tearDown(); } } From c39ded21d2a2388d82e31448639517af0e2a2637 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 16 Jul 2015 12:56:22 +0200 Subject: [PATCH 05/12] initialize unused (for now) ttl field to 0 --- 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 aee66bca745..d7cbf5d9943 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -38,7 +38,7 @@ class DBLockingProvider extends AbstractLockingProvider { } protected function initLockField($path) { - $this->connection->insertIfNotExist('*PREFIX*file_locks', ['path' => $path, 'lock' => 0], ['path']); + $this->connection->insertIfNotExist('*PREFIX*file_locks', ['path' => $path, 'lock' => 0, 'ttl' => 0], ['path']); } /** From 132a564a217d38a9ac66e26a00d0c1181bd1257a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 3 Aug 2015 15:46:19 +0200 Subject: [PATCH 06/12] rename path field to key --- db_structure.xml | 9 +++++---- lib/private/lock/dblockingprovider.php | 21 +++++++++++++-------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/db_structure.xml b/db_structure.xml index 5c2b26e5f15..5633d1537c0 100644 --- a/db_structure.xml +++ b/db_structure.xml @@ -1183,7 +1183,7 @@ *dbprefix*file_locks @@ -1208,7 +1208,7 @@ - path + key text true 64 @@ -1217,6 +1217,7 @@ ttl integer + -1 true 4 @@ -1233,9 +1234,9 @@ true - lock_path_index + lock_key_index - path + key ascending diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index d7cbf5d9943..60d516e17c0 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -24,6 +24,9 @@ namespace OC\Lock; use OCP\IDBConnection; use OCP\Lock\LockedException; +/** + * Locking provider that stores the locks in the database + */ class DBLockingProvider extends AbstractLockingProvider { /** * @var \OCP\IDBConnection @@ -38,7 +41,7 @@ class DBLockingProvider extends AbstractLockingProvider { } protected function initLockField($path) { - $this->connection->insertIfNotExist('*PREFIX*file_locks', ['path' => $path, 'lock' => 0, 'ttl' => 0], ['path']); + $this->connection->insertIfNotExist('*PREFIX*file_locks', ['key' => $path, 'lock' => 0, 'ttl' => 0], ['key']); } /** @@ -47,7 +50,7 @@ class DBLockingProvider extends AbstractLockingProvider { * @return bool */ public function isLocked($path, $type) { - $query = $this->connection->prepare('SELECT `lock` from `*PREFIX*file_locks` WHERE `path` = ?'); + $query = $this->connection->prepare('SELECT `lock` from `*PREFIX*file_locks` WHERE `key` = ?'); $query->execute([$path]); $lockValue = (int)$query->fetchColumn(); if ($type === self::LOCK_SHARED) { @@ -65,18 +68,20 @@ class DBLockingProvider extends AbstractLockingProvider { * @throws \OCP\Lock\LockedException */ public function acquireLock($path, $type) { + $this->connection->beginTransaction(); $this->initLockField($path); if ($type === self::LOCK_SHARED) { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1 WHERE `path` = ? AND `lock` >= 0', + 'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` + 1 WHERE `key` = ? AND `lock` >= 0', [$path] ); } else { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 0', + 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `key` = ? AND `lock` = 0', [$path] ); } + $this->connection->commit(); if ($result !== 1) { throw new LockedException($path); } @@ -91,12 +96,12 @@ class DBLockingProvider extends AbstractLockingProvider { $this->initLockField($path); if ($type === self::LOCK_SHARED) { $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` - 1 WHERE `path` = ? AND `lock` > 0', + 'UPDATE `*PREFIX*file_locks` SET `lock` = `lock` - 1 WHERE `key` = ? AND `lock` > 0', [$path] ); } else { $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = 0 WHERE `path` = ? AND `lock` = -1', + 'UPDATE `*PREFIX*file_locks` SET `lock` = 0 WHERE `key` = ? AND `lock` = -1', [$path] ); } @@ -114,12 +119,12 @@ class DBLockingProvider extends AbstractLockingProvider { $this->initLockField($path); if ($targetType === self::LOCK_SHARED) { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = 1 WHERE `path` = ? AND `lock` = -1', + 'UPDATE `*PREFIX*file_locks` SET `lock` = 1 WHERE `key` = ? AND `lock` = -1', [$path] ); } else { $result = $this->connection->executeUpdate( - 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `path` = ? AND `lock` = 1', + 'UPDATE `*PREFIX*file_locks` SET `lock` = -1 WHERE `key` = ? AND `lock` = 1', [$path] ); } From cd205249e4deb397235dde90e707135fcc85d878 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 3 Aug 2015 15:46:23 +0200 Subject: [PATCH 07/12] more phpdoc --- lib/private/lock/abstractlockingprovider.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/private/lock/abstractlockingprovider.php b/lib/private/lock/abstractlockingprovider.php index 03b4912ea13..c9c3e08994e 100644 --- a/lib/private/lock/abstractlockingprovider.php +++ b/lib/private/lock/abstractlockingprovider.php @@ -23,6 +23,10 @@ namespace OC\Lock; use OCP\Lock\ILockingProvider; +/** + * Base locking provider that keeps track of locks acquired during the current request + * to release any left over locks at the end of the request + */ abstract class AbstractLockingProvider implements ILockingProvider { protected $acquiredLocks = [ 'shared' => [], @@ -30,6 +34,8 @@ abstract class AbstractLockingProvider implements ILockingProvider { ]; /** + * Mark a locally acquired lock + * * @param string $path * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE */ @@ -45,6 +51,8 @@ abstract class AbstractLockingProvider implements ILockingProvider { } /** + * Mark a release of a locally acquired lock + * * @param string $path * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE */ @@ -59,7 +67,7 @@ abstract class AbstractLockingProvider implements ILockingProvider { } /** - * Change the type of an existing lock + * Change the type of an existing tracked lock * * @param string $path * @param int $targetType self::LOCK_SHARED or self::LOCK_EXCLUSIVE From 9729e67e3d2dec127e2d9e3d47d01d4de806b302 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 3 Aug 2015 16:02:24 +0200 Subject: [PATCH 08/12] more phpdoc --- lib/private/lock/abstractlockingprovider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/lock/abstractlockingprovider.php b/lib/private/lock/abstractlockingprovider.php index c9c3e08994e..eb86be68500 100644 --- a/lib/private/lock/abstractlockingprovider.php +++ b/lib/private/lock/abstractlockingprovider.php @@ -86,7 +86,7 @@ abstract class AbstractLockingProvider implements ILockingProvider { } /** - * release all lock acquired by this instance + * release all lock acquired by this instance which were marked using the mark* methods */ public function releaseAll() { foreach ($this->acquiredLocks['shared'] as $path => $count) { From 06065189d768e5b33cb4798403db34b12cb60ef7 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 10 Aug 2015 14:13:40 +0200 Subject: [PATCH 09/12] cleanup empty locks --- lib/private/lock/dblockingprovider.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index 60d516e17c0..5480a6e53af 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -105,6 +105,7 @@ class DBLockingProvider extends AbstractLockingProvider { [$path] ); } + $this->markRelease($path, $type); } @@ -133,4 +134,17 @@ 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 58e96e53b039365751eb8c4a2d511fcfcf507891 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 10 Aug 2015 14:15:44 +0200 Subject: [PATCH 10/12] add method to check if we're inside a transaction --- lib/private/appframework/db/db.php | 9 +++++++++ lib/private/db/connection.php | 10 ++++++++++ lib/public/idbconnection.php | 8 ++++++++ 3 files changed, 27 insertions(+) diff --git a/lib/private/appframework/db/db.php b/lib/private/appframework/db/db.php index cde85831687..8e3fa6e4197 100644 --- a/lib/private/appframework/db/db.php +++ b/lib/private/appframework/db/db.php @@ -153,6 +153,15 @@ class Db implements IDb { $this->connection->beginTransaction(); } + /** + * Check if a transaction is active + * + * @return bool + */ + public function inTransaction() { + return $this->connection->inTransaction(); + } + /** * Commit the database changes done during a transaction that is in progress */ diff --git a/lib/private/db/connection.php b/lib/private/db/connection.php index def3f2fd120..4d33cd968af 100644 --- a/lib/private/db/connection.php +++ b/lib/private/db/connection.php @@ -291,4 +291,14 @@ class Connection extends \Doctrine\DBAL\Connection implements IDBConnection { protected function replaceTablePrefix($statement) { return str_replace( '*PREFIX*', $this->tablePrefix, $statement ); } + + /** + * Check if a transaction is active + * + * @return bool + * @since 8.2.0 + */ + public function inTransaction() { + return $this->getTransactionNestingLevel() > 0; + } } diff --git a/lib/public/idbconnection.php b/lib/public/idbconnection.php index 0d04c43d73e..6a4373583fa 100644 --- a/lib/public/idbconnection.php +++ b/lib/public/idbconnection.php @@ -114,6 +114,14 @@ interface IDBConnection { */ public function beginTransaction(); + /** + * Check if a transaction is active + * + * @return bool + * @since 8.2.0 + */ + public function inTransaction(); + /** * Commit the database changes done during a transaction that is in progress * @since 6.0.0 From d979e54030b0069572affe173c06680597cc831d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 10 Aug 2015 14:39:34 +0200 Subject: [PATCH 11/12] log a warning while trying to acquire a db lock from within a transaction --- lib/private/lock/dblockingprovider.php | 16 ++++++++++++++-- lib/private/server.php | 8 ++++---- tests/lib/lock/dblockingprovider.php | 2 +- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/private/lock/dblockingprovider.php b/lib/private/lock/dblockingprovider.php index 5480a6e53af..5241a0440b3 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -22,6 +22,7 @@ namespace OC\Lock; use OCP\IDBConnection; +use OCP\ILogger; use OCP\Lock\LockedException; /** @@ -34,10 +35,17 @@ class DBLockingProvider extends AbstractLockingProvider { private $connection; /** - * @param \OCP\IDBConnection $connection + * @var \OCP\ILogger */ - public function __construct(IDBConnection $connection) { + private $logger; + + /** + * @param \OCP\IDBConnection $connection + * @param \OCP\ILogger $logger + */ + public function __construct(IDBConnection $connection, ILogger $logger) { $this->connection = $connection; + $this->logger = $logger; } protected function initLockField($path) { @@ -68,6 +76,10 @@ class DBLockingProvider extends AbstractLockingProvider { * @throws \OCP\Lock\LockedException */ public function acquireLock($path, $type) { + if ($this->connection->inTransaction()){ + $this->logger->warning('Trying to acquire a lock while inside a transition'); + } + $this->connection->beginTransaction(); $this->initLockField($path); if ($type === self::LOCK_SHARED) { diff --git a/lib/private/server.php b/lib/private/server.php index 51e70405432..be2cfc2cf88 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -432,10 +432,10 @@ class Server extends SimpleContainer implements IServerContainer { /** @var \OC\Memcache\Factory $memcacheFactory */ $memcacheFactory = $c->getMemCacheFactory(); $memcache = $memcacheFactory->createLocking('lock'); - if (!($memcache instanceof \OC\Memcache\NullCache)) { - return new MemcacheLockingProvider($memcache); - } - return new DBLockingProvider($c->getDatabaseConnection()); +// if (!($memcache instanceof \OC\Memcache\NullCache)) { +// return new MemcacheLockingProvider($memcache); +// } + return new DBLockingProvider($c->getDatabaseConnection(), $c->getLogger()); } return new NoopLockingProvider(); }); diff --git a/tests/lib/lock/dblockingprovider.php b/tests/lib/lock/dblockingprovider.php index 4818ef1ceca..fd6550d9c47 100644 --- a/tests/lib/lock/dblockingprovider.php +++ b/tests/lib/lock/dblockingprovider.php @@ -33,7 +33,7 @@ class DBLockingProvider extends LockingProvider { */ protected function getInstance() { $this->connection = \OC::$server->getDatabaseConnection(); - return new \OC\Lock\DBLockingProvider($this->connection); + return new \OC\Lock\DBLockingProvider($this->connection, \OC::$server->getLogger()); } public function tearDown() { From 6f6a5f6c2981cd046abc0530c4b6a222e67f17a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Tue, 25 Aug 2015 14:31:21 +0200 Subject: [PATCH 12/12] Adding path to log message --- 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 5241a0440b3..f3e684d0b4d 100644 --- a/lib/private/lock/dblockingprovider.php +++ b/lib/private/lock/dblockingprovider.php @@ -77,7 +77,7 @@ class DBLockingProvider extends AbstractLockingProvider { */ public function acquireLock($path, $type) { if ($this->connection->inTransaction()){ - $this->logger->warning('Trying to acquire a lock while inside a transition'); + $this->logger->warning("Trying to acquire a lock for '$path' while inside a transition"); } $this->connection->beginTransaction();