Merge pull request #7528 from nupplaphil/bug/fix_locks

Fix Locks

src/Core/Lock/DatabaseLock.php

@@ -82,7 +82,11 @@ class DatabaseLock extends Lock
 			$where = ['name' => $key, 'pid' => $this->pid];
 		}
 
-		$return = $this->dba->delete('locks', $where);
+		if ($this->dba->exists('locks', $where)) {
+			$return = $this->dba->delete('locks', $where);
+		} else {
+			$return = false;
+		}
 
 		$this->markRelease($key);
 
@@ -105,7 +109,7 @@ class DatabaseLock extends Lock
 
 		$this->acquiredLocks = [];
 
-		return $return;
+		return $return && $success;
 	}
 
 	/**

src/Core/Lock/SemaphoreLock.php

@@ -20,27 +20,17 @@ class SemaphoreLock extends Lock
 	 */
 	private static function semaphoreKey($key)
 	{
-		$file = self::keyToFile($key);
+		$success = true;
 
-		if (!file_exists($file)) {
-			file_put_contents($file, $key);
-		}
-
-		return ftok($file, 'f');
-	}
-
-	/**
-	 * Returns the full path to the semaphore file
-	 *
-	 * @param string $key The key of the semaphore
-	 *
-	 * @return string The full path
-	 */
-	private static function keyToFile($key)
-	{
 		$temp = get_temppath();
 
-		return $temp . '/' . $key . '.sem';
+		$file = $temp . '/' . $key . '.sem';
+
+		if (!file_exists($file)) {
+			$success = !empty(file_put_contents($file, $key));
+		}
+
+		return $success ? ftok($file, 'f') : false;
 	}
 
 	/**
@@ -61,6 +51,9 @@ class SemaphoreLock extends Lock
 
 	/**
 	 * (@inheritdoc)
+	 *
+	 * @param bool $override not necessary parameter for semaphore locks since the lock lives as long as the execution
+	 *                       of the using function
 	 */
 	public function releaseLock($key, $override = false)
 	{
@@ -69,18 +62,11 @@ class SemaphoreLock extends Lock
 		if (!empty(self::$semaphore[$key])) {
 			try {
 				$success = @sem_release(self::$semaphore[$key]);
-				if (file_exists(self::keyToFile($key)) && $success) {
-					$success = unlink(self::keyToFile($key));
-				}
 				unset(self::$semaphore[$key]);
 				$this->markRelease($key);
 			} catch (\Exception $exception) {
 				$success = false;
 			}
-		} else if ($override) {
-			if ($this->acquireLock($key)) {
-				$success = $this->releaseLock($key, true);
-			}
 		}
 
 		return $success;
@@ -107,16 +93,23 @@ class SemaphoreLock extends Lock
 	 */
 	public function getLocks(string $prefix = '')
 	{
-		$temp = get_temppath();
-		$locks = [];
-		foreach (glob(sprintf('%s/%s*.sem', $temp, $prefix)) as $lock) {
-			$lock = pathinfo($lock, PATHINFO_FILENAME);
-			if(sem_get(self::semaphoreKey($lock))) {
-				$locks[] = $lock;
-			}
-		}
+		// We can just return our own semaphore keys, since we don't know
+		// the state of other semaphores, even if the .sem files exists
+		$keys = array_keys(self::$semaphore);
 
-		return $locks;
+		if (empty($prefix)) {
+			return $keys;
+		} else {
+			$result = [];
+
+			foreach ($keys as $key) {
+				if (strpos($key, $prefix) === 0) {
+					array_push($result, $key);
+				}
+			}
+
+			return $result;
+		}
 	}
 
 	/**
@@ -124,16 +117,8 @@ class SemaphoreLock extends Lock
 	 */
 	public function releaseAll($override = false)
 	{
-		$success = parent::releaseAll($override);
-
-		$temp = get_temppath();
-		foreach (glob(sprintf('%s/*.sem', $temp)) as $lock) {
-			$lock = pathinfo($lock, PATHINFO_FILENAME);
-			if (!$this->releaseLock($lock, true)) {
-				$success = false;
-			}
-		}
-
-		return $success;
+		// Semaphores are just alive during a run, so there is no need to release
+		// You can just release your own locks
+		return parent::releaseAll($override);
 	}
 }

src/Factory/LockFactory.php

@@ -7,7 +7,6 @@ use Friendica\Core\Cache\IMemoryCache;
 use Friendica\Core\Config\Configuration;
 use Friendica\Core\Lock;
 use Friendica\Database\Database;
-use Friendica\Util\Profiler;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -39,17 +38,12 @@ class LockFactory
 	 */
 	private $cacheFactory;
 
-	/**
-	 * @var Profiler The optional profiler if the cached should be profiled
-	 */
-	private $profiler;
-
 	/**
 	 * @var LoggerInterface The Friendica Logger
 	 */
 	private $logger;
 
-	public function __construct(CacheFactory $cacheFactory, Configuration $config, Database $dba, Profiler $profiler, LoggerInterface $logger)
+	public function __construct(CacheFactory $cacheFactory, Configuration $config, Database $dba, LoggerInterface $logger)
 	{
 		$this->cacheFactory = $cacheFactory;
 		$this->config       = $config;

tests/src/Core/Lock/LockTest.php

@@ -190,4 +190,13 @@ abstract class LockTest extends MockedTest
 		$this->assertFalse($this->instance->isLocked('foo'));
 		$this->assertFalse($this->instance->isLocked('bar'));
 	}
+
+	/**
+	 * Test if releasing a non-existing lock doesn't throw errors
+	 */
+	public function testReleaseLockWithoutLock()
+	{
+		$this->assertFalse($this->instance->isLocked('wrongLock'));
+		$this->assertFalse($this->instance->releaseLock('wrongLock'));
+	}
 }

tests/src/Core/Lock/SemaphoreLockTest.php

@@ -41,4 +41,49 @@ class SemaphoreLockTest extends LockTest
 		// Semaphore doesn't work with TTL
 		return true;
 	}
+
+	/**
+	 * Test if semaphore locking works even when trying to release locks, where the file exists
+	 * but it shouldn't harm locking
+	 */
+	public function testMissingFileNotOverriding()
+	{
+		$file = get_temppath() . '/test.sem';
+		touch($file);
+
+		$this->assertTrue(file_exists($file));
+		$this->assertFalse($this->instance->releaseLock('test', false));
+		$this->assertTrue(file_exists($file));
+	}
+
+	/**
+	 * Test overriding semaphore release with already set semaphore
+	 * This test proves that semaphore locks cannot get released by other instances except themselves
+	 *
+	 * Check for Bug https://github.com/friendica/friendica/issues/7298#issuecomment-521996540
+	 * @see https://github.com/friendica/friendica/issues/7298#issuecomment-521996540
+	 */
+	public function testMissingFileOverriding()
+	{
+		$file = get_temppath() . '/test.sem';
+		touch($file);
+
+		$this->assertTrue(file_exists($file));
+		$this->assertFalse($this->instance->releaseLock('test', true));
+		$this->assertTrue(file_exists($file));
+	}
+
+	/**
+	 * Test acquire lock even the semaphore file exists, but isn't used
+	 */
+	public function testOverrideSemFile()
+	{
+		$file = get_temppath() . '/test.sem';
+		touch($file);
+
+		$this->assertTrue(file_exists($file));
+		$this->assertTrue($this->instance->acquireLock('test'));
+		$this->assertTrue($this->instance->isLocked('test'));
+		$this->assertTrue($this->instance->releaseLock('test'));
+	}
 }