From c803dcb6c5f808ac8f962e9bd5e631a91655cbc7 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Sat, 17 Aug 2019 19:33:36 +0200 Subject: [PATCH 1/3] Fix Locks - Wrong return of lock releasing with DBA provider - It's not possible to maintain Semaphore locks, since they aren't accessible by other processes Should solve https://github.com/friendica/friendica/issues/7298#issuecomment-521996540 --- src/Core/Lock/DatabaseLock.php | 8 ++- src/Core/Lock/SemaphoreLock.php | 75 +++++++++-------------- tests/src/Core/Lock/LockTest.php | 9 +++ tests/src/Core/Lock/SemaphoreLockTest.php | 30 ++++++++- 4 files changed, 74 insertions(+), 48 deletions(-) diff --git a/src/Core/Lock/DatabaseLock.php b/src/Core/Lock/DatabaseLock.php index 07cf88c49..cecdc3966 100644 --- a/src/Core/Lock/DatabaseLock.php +++ b/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; } /** diff --git a/src/Core/Lock/SemaphoreLock.php b/src/Core/Lock/SemaphoreLock.php index 0f41f9f30..d5153d432 100644 --- a/src/Core/Lock/SemaphoreLock.php +++ b/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); } } diff --git a/tests/src/Core/Lock/LockTest.php b/tests/src/Core/Lock/LockTest.php index dd38172b3..24efeaab8 100644 --- a/tests/src/Core/Lock/LockTest.php +++ b/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')); + } } diff --git a/tests/src/Core/Lock/SemaphoreLockTest.php b/tests/src/Core/Lock/SemaphoreLockTest.php index 52c5aaa5b..dd696d4ae 100644 --- a/tests/src/Core/Lock/SemaphoreLockTest.php +++ b/tests/src/Core/Lock/SemaphoreLockTest.php @@ -22,7 +22,7 @@ class SemaphoreLockTest extends LockTest $configMock ->shouldReceive('get') ->with('system', 'temppath', NULL, false) - ->andReturn('/tmp/'); + ->andReturn('/tmp'); $dice->shouldReceive('create')->with(Configuration::class)->andReturn($configMock); // @todo Because "get_temppath()" is using static methods, we have to initialize the BaseObject @@ -41,4 +41,32 @@ class SemaphoreLockTest extends LockTest // Semaphore doesn't work with TTL return true; } + + /** + * Test if semaphore locking works even for + */ + public function testMissingFileNotOverriding() + { + $file = get_temppath() . '/test.sem'; + + $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'; + + $this->assertTrue(file_exists($file)); + $this->assertFalse($this->instance->releaseLock('test', true)); + $this->assertTrue(file_exists($file)); + } } From 1237ac1062b8d76d1b002a4c4ac8163f7ea085e6 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Sat, 17 Aug 2019 19:38:51 +0200 Subject: [PATCH 2/3] enhance semaphore lock testing --- tests/src/Core/Lock/SemaphoreLockTest.php | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/tests/src/Core/Lock/SemaphoreLockTest.php b/tests/src/Core/Lock/SemaphoreLockTest.php index dd696d4ae..a2291b2c2 100644 --- a/tests/src/Core/Lock/SemaphoreLockTest.php +++ b/tests/src/Core/Lock/SemaphoreLockTest.php @@ -22,7 +22,7 @@ class SemaphoreLockTest extends LockTest $configMock ->shouldReceive('get') ->with('system', 'temppath', NULL, false) - ->andReturn('/tmp'); + ->andReturn('/tmp/'); $dice->shouldReceive('create')->with(Configuration::class)->andReturn($configMock); // @todo Because "get_temppath()" is using static methods, we have to initialize the BaseObject @@ -43,11 +43,13 @@ class SemaphoreLockTest extends LockTest } /** - * Test if semaphore locking works even for + * 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)); @@ -64,9 +66,24 @@ class SemaphoreLockTest extends LockTest 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')); + } } From 46655eac7022d24381863a0765e200be933ebe56 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Sat, 17 Aug 2019 19:41:59 +0200 Subject: [PATCH 3/3] Remove unnecessary parameter --- src/Factory/LockFactory.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Factory/LockFactory.php b/src/Factory/LockFactory.php index fef6708d2..cee01fe1e 100644 --- a/src/Factory/LockFactory.php +++ b/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;