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

diff --git a/src/Core/Lock/DatabaseLock.php b/src/Core/Lock/DatabaseLock.php
index 07cf88c4948c68716e2114dad031de13aad4caf0..cecdc3966e41d9a6a1a0abfe889d6acdc8c439ee 100644 (file)
--- 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 0f41f9f309556cf7355016044ce7ba85294d3f93..d5153d432e46b0ccf68a27f6e18e02c1429fcb48 100644 (file)
--- 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);
-               }
+               $temp = get_temppath();
 
-               return ftok($file, 'f');
-       }
+               $file = $temp . '/' . $key . '.sem';
 
-       /**
-        * 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();
+               if (!file_exists($file)) {
+                       $success = !empty(file_put_contents($file, $key));
+               }
 
-               return $temp . '/' . $key . '.sem';
+               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);
+
+               if (empty($prefix)) {
+                       return $keys;
+               } else {
+                       $result = [];
+
+                       foreach ($keys as $key) {
+                               if (strpos($key, $prefix) === 0) {
+                                       array_push($result, $key);
+                               }
                        }
-               }
 
-               return $locks;
+                       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 dd38172b38d0b0c7e4d225dd119cfd819998fc50..24efeaab869823d7f6fa2c5b7cf179798d936502 100644 (file)
--- 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 52c5aaa5b88cb7b48653de9b9b20538cde588c64..dd696d4aefa1f84b6bb228692ef0d3c004597b70 100644 (file)
--- 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));
+       }
 }