Merge pull request #9134 from nextcloud/db-locks-cli

dont keep shared database locks when running cli scripts

lib/private/DB/QueryBuilder/FunctionBuilder/FunctionBuilder.php

@@ -63,4 +63,12 @@ class FunctionBuilder implements IFunctionBuilder {
 	public function lower($field) {
 		return new QueryFunction('LOWER(' . $this->helper->quoteColumnName($field) . ')');
 	}
+
+	public function add($x, $y) {
+		return new QueryFunction($this->helper->quoteColumnName($x) . ' + ' . $this->helper->quoteColumnName($y));
+	}
+
+	public function subtract($x, $y) {
+		return new QueryFunction($this->helper->quoteColumnName($x) . ' - ' . $this->helper->quoteColumnName($y));
+	}
 }

lib/private/Lock/DBLockingProvider.php

@@ -55,6 +55,11 @@ class DBLockingProvider extends AbstractLockingProvider {
 
 	private $sharedLocks = [];
 
+	/**
+	 * @var bool
+	 */
+	private $cacheSharedLocks;
+
 	/**
 	 * Check if we have an open shared lock for a path
 	 *
@@ -73,8 +78,10 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 */
 	protected function markAcquire(string $path, int $type) {
 		parent::markAcquire($path, $type);
-		if ($type === self::LOCK_SHARED) {
-			$this->sharedLocks[$path] = true;
+		if ($this->cacheSharedLocks) {
+			if ($type === self::LOCK_SHARED) {
+				$this->sharedLocks[$path] = true;
+			}
 		}
 	}
 
@@ -86,10 +93,12 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 */
 	protected function markChange(string $path, int $targetType) {
 		parent::markChange($path, $targetType);
-		if ($targetType === self::LOCK_SHARED) {
-			$this->sharedLocks[$path] = true;
-		} else if ($targetType === self::LOCK_EXCLUSIVE) {
-			$this->sharedLocks[$path] = false;
+		if ($this->cacheSharedLocks) {
+			if ($targetType === self::LOCK_SHARED) {
+				$this->sharedLocks[$path] = true;
+			} else if ($targetType === self::LOCK_EXCLUSIVE) {
+				$this->sharedLocks[$path] = false;
+			}
 		}
 	}
 
@@ -98,12 +107,20 @@ class DBLockingProvider extends AbstractLockingProvider {
 	 * @param \OCP\ILogger $logger
 	 * @param \OCP\AppFramework\Utility\ITimeFactory $timeFactory
 	 * @param int $ttl
+	 * @param bool $cacheSharedLocks
 	 */
-	public function __construct(IDBConnection $connection, ILogger $logger, ITimeFactory $timeFactory, int $ttl = 3600) {
+	public function __construct(
+		IDBConnection $connection,
+		ILogger $logger,
+		ITimeFactory $timeFactory,
+		int $ttl = 3600,
+		$cacheSharedLocks = true
+	) {
 		$this->connection = $connection;
 		$this->logger = $logger;
 		$this->timeFactory = $timeFactory;
 		$this->ttl = $ttl;
+		$this->cacheSharedLocks = $cacheSharedLocks;
 	}
 
 	/**
@@ -203,6 +220,13 @@ class DBLockingProvider extends AbstractLockingProvider {
 				'UPDATE `*PREFIX*file_locks` SET `lock` = 0 WHERE `key` = ? AND `lock` = -1',
 				[$path]
 			);
+		} else if (!$this->cacheSharedLocks) {
+			$query = $this->connection->getQueryBuilder();
+			$query->update('file_locks')
+				->set('lock', $query->func()->subtract('lock', $query->createNamedParameter(1)))
+				->where($query->expr()->eq('key', $query->createNamedParameter($path)))
+				->andWhere($query->expr()->gt('lock', $query->createNamedParameter(0)));
+			$query->execute();
 		}
 	}
 
@@ -256,11 +280,15 @@ class DBLockingProvider extends AbstractLockingProvider {
 
 	/**
 	 * release all lock acquired by this instance which were marked using the mark* methods
+	 *
 	 * @suppress SqlInjectionChecker
 	 */
 	public function releaseAll() {
 		parent::releaseAll();
 
+		if (!$this->cacheSharedLocks) {
+			return;
+		}
 		// since we keep shared locks we need to manually clean those
 		$lockedPaths = array_keys($this->sharedLocks);
 		$lockedPaths = array_filter($lockedPaths, function ($path) {

lib/private/Server.php

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

lib/public/DB/QueryBuilder/IFunctionBuilder.php

@@ -80,4 +80,20 @@ interface IFunctionBuilder {
 	 * @since 14.0.0
 	 */
 	public function lower($field);
+
+	/**
+	 * @param mixed $x The first input field or number
+	 * @param mixed $y The second input field or number
+	 * @return IQueryFunction
+	 * @since 14.0.0
+	 */
+	public function add($x, $y);
+
+	/**
+	 * @param mixed $x The first input field or number
+	 * @param mixed $y The second input field or number
+	 * @return IQueryFunction
+	 * @since 14.0.0
+	 */
+	public function subtract($x, $y);
 }

tests/lib/DB/QueryBuilder/FunctionBuilderTest.php

@@ -21,6 +21,7 @@
 namespace Test\DB\QueryBuilder;
 
 use OC\DB\QueryBuilder\Literal;
+use OCP\DB\QueryBuilder\IQueryBuilder;
 use Test\TestCase;
 
 /**
@@ -89,4 +90,24 @@ class FunctionBuilderTest extends TestCase {
 
 		$this->assertEquals('foobar', $query->execute()->fetchColumn());
 	}
+
+	public function testAdd() {
+		$query = $this->connection->getQueryBuilder();
+
+		$query->select($query->func()->add($query->createNamedParameter(2, IQueryBuilder::PARAM_INT), new Literal(1)));
+		$query->from('appconfig')
+			->setMaxResults(1);
+
+		$this->assertEquals(3, $query->execute()->fetchColumn());
+	}
+
+	public function testSubtract() {
+		$query = $this->connection->getQueryBuilder();
+
+		$query->select($query->func()->subtract($query->createNamedParameter(2, IQueryBuilder::PARAM_INT), new Literal(1)));
+		$query->from('appconfig')
+			->setMaxResults(1);
+
+		$this->assertEquals(1, $query->execute()->fetchColumn());
+	}
 }

tests/lib/Lock/DBLockingProviderTest.php

@@ -40,14 +40,14 @@ class DBLockingProviderTest extends LockingProvider {
 	/**
 	 * @var \OCP\IDBConnection
 	 */
-	private $connection;
+	protected $connection;
 
 	/**
 	 * @var \OCP\AppFramework\Utility\ITimeFactory
 	 */
-	private $timeFactory;
+	protected $timeFactory;
 
-	private $currentTime;
+	protected $currentTime;
 
 	public function setUp() {
 		$this->currentTime = time();
@@ -96,4 +96,31 @@ class DBLockingProviderTest extends LockingProvider {
 		$query->execute();
 		return $query->fetchColumn();
 	}
+
+	protected function getLockValue($key) {
+		$query = $this->connection->getQueryBuilder();
+		$query->select('lock')
+			->from('file_locks')
+			->where($query->expr()->eq('key', $query->createNamedParameter($key)));
+		return $query->execute()->fetchColumn();
+	}
+
+	public function testDoubleShared() {
+		$this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED);
+		$this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED);
+
+		$this->assertEquals(1, $this->getLockValue('foo'));
+
+		$this->instance->releaseLock('foo', ILockingProvider::LOCK_SHARED);
+
+		$this->assertEquals(1, $this->getLockValue('foo'));
+
+		$this->instance->releaseLock('foo', ILockingProvider::LOCK_SHARED);
+
+		$this->assertEquals(1, $this->getLockValue('foo'));
+
+		$this->instance->releaseAll();
+
+		$this->assertEquals(0, $this->getLockValue('foo'));
+	}
 }

tests/lib/Lock/NonCachingDBLockingProviderTest.php

@@ -0,0 +1,54 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 Robin Appelman <robin@icewind.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * 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
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Test\Lock;
+
+use OCP\Lock\ILockingProvider;
+
+/**
+ * @group DB
+ *
+ * @package Test\Lock
+ */
+class NonCachingDBLockingProviderTest extends DBLockingProviderTest {
+	/**
+	 * @return \OCP\Lock\ILockingProvider
+	 */
+	protected function getInstance() {
+		$this->connection = \OC::$server->getDatabaseConnection();
+		return new \OC\Lock\DBLockingProvider($this->connection, \OC::$server->getLogger(), $this->timeFactory, 3600, false);
+	}
+
+	public function testDoubleShared() {
+		$this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED);
+		$this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED);
+
+		$this->assertEquals(2, $this->getLockValue('foo'));
+
+		$this->instance->releaseLock('foo', ILockingProvider::LOCK_SHARED);
+
+		$this->assertEquals(1, $this->getLockValue('foo'));
+
+		$this->instance->releaseLock('foo', ILockingProvider::LOCK_SHARED);
+
+		$this->assertEquals(0, $this->getLockValue('foo'));
+	}
+}