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
This commit is contained in:
Philipp Holzer 2019-08-17 19:33:36 +02:00
parent e8561b480b
commit c803dcb6c5
No known key found for this signature in database
GPG key ID: D8365C3D36B77D90
4 changed files with 74 additions and 48 deletions

View file

@ -82,7 +82,11 @@ class DatabaseLock extends Lock
$where = ['name' => $key, 'pid' => $this->pid]; $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); $this->markRelease($key);
@ -105,7 +109,7 @@ class DatabaseLock extends Lock
$this->acquiredLocks = []; $this->acquiredLocks = [];
return $return; return $return && $success;
} }
/** /**

View file

@ -20,27 +20,17 @@ class SemaphoreLock extends Lock
*/ */
private static function semaphoreKey($key) 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(); $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) * (@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) public function releaseLock($key, $override = false)
{ {
@ -69,18 +62,11 @@ class SemaphoreLock extends Lock
if (!empty(self::$semaphore[$key])) { if (!empty(self::$semaphore[$key])) {
try { try {
$success = @sem_release(self::$semaphore[$key]); $success = @sem_release(self::$semaphore[$key]);
if (file_exists(self::keyToFile($key)) && $success) {
$success = unlink(self::keyToFile($key));
}
unset(self::$semaphore[$key]); unset(self::$semaphore[$key]);
$this->markRelease($key); $this->markRelease($key);
} catch (\Exception $exception) { } catch (\Exception $exception) {
$success = false; $success = false;
} }
} else if ($override) {
if ($this->acquireLock($key)) {
$success = $this->releaseLock($key, true);
}
} }
return $success; return $success;
@ -107,16 +93,23 @@ class SemaphoreLock extends Lock
*/ */
public function getLocks(string $prefix = '') public function getLocks(string $prefix = '')
{ {
$temp = get_temppath(); // We can just return our own semaphore keys, since we don't know
$locks = []; // the state of other semaphores, even if the .sem files exists
foreach (glob(sprintf('%s/%s*.sem', $temp, $prefix)) as $lock) { $keys = array_keys(self::$semaphore);
$lock = pathinfo($lock, PATHINFO_FILENAME);
if(sem_get(self::semaphoreKey($lock))) {
$locks[] = $lock;
}
}
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) public function releaseAll($override = false)
{ {
$success = parent::releaseAll($override); // Semaphores are just alive during a run, so there is no need to release
// You can just release your own locks
$temp = get_temppath(); return parent::releaseAll($override);
foreach (glob(sprintf('%s/*.sem', $temp)) as $lock) {
$lock = pathinfo($lock, PATHINFO_FILENAME);
if (!$this->releaseLock($lock, true)) {
$success = false;
}
}
return $success;
} }
} }

View file

@ -190,4 +190,13 @@ abstract class LockTest extends MockedTest
$this->assertFalse($this->instance->isLocked('foo')); $this->assertFalse($this->instance->isLocked('foo'));
$this->assertFalse($this->instance->isLocked('bar')); $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'));
}
} }

View file

@ -22,7 +22,7 @@ class SemaphoreLockTest extends LockTest
$configMock $configMock
->shouldReceive('get') ->shouldReceive('get')
->with('system', 'temppath', NULL, false) ->with('system', 'temppath', NULL, false)
->andReturn('/tmp/'); ->andReturn('/tmp');
$dice->shouldReceive('create')->with(Configuration::class)->andReturn($configMock); $dice->shouldReceive('create')->with(Configuration::class)->andReturn($configMock);
// @todo Because "get_temppath()" is using static methods, we have to initialize the BaseObject // @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 // Semaphore doesn't work with TTL
return true; 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));
}
} }