Merge pull request #6808 from nupplaphil/issue/redis_lock

Redis acquireLock() issue

src/Core/Cache/RedisCacheDriver.php

@@ -2,9 +2,8 @@
 
 namespace Friendica\Core\Cache;
 
-use Friendica\Core\Cache;
-
 use Exception;
+use Friendica\Core\Cache;
 use Redis;
 
 /**
@@ -152,7 +151,7 @@ class RedisCacheDriver extends AbstractCacheDriver implements IMemoryCacheDriver
 					->exec();
 			} else {
 				$result = $this->redis->multi()
-					->set($cachekey, $newValue)
+					->set($cachekey, $newCached)
 					->exec();
 			}
 			return $result !== false;

src/Core/Lock/AbstractLockDriver.php

@@ -23,7 +23,8 @@ abstract class AbstractLockDriver extends BaseObject implements ILockDriver
 	 * @param string key The Name of the lock
 	 * @return bool      Returns true if the lock is set
 	 */
-	protected function hasAcquiredLock($key) {
+	protected function hasAcquiredLock($key)
+	{
 		return isset($this->acquireLock[$key]) && $this->acquiredLocks[$key] === true;
 	}
 
@@ -32,7 +33,8 @@ abstract class AbstractLockDriver extends BaseObject implements ILockDriver
 	 *
 	 * @param string $key The Name of the lock
 	 */
-	protected function markAcquire($key) {
+	protected function markAcquire($key)
+	{
 		$this->acquiredLocks[$key] = true;
 	}
 
@@ -41,18 +43,26 @@ abstract class AbstractLockDriver extends BaseObject implements ILockDriver
 	 *
 	 * @param string $key The Name of the lock
 	 */
-	protected function markRelease($key) {
+	protected function markRelease($key)
+	{
 		unset($this->acquiredLocks[$key]);
 	}
 
 	/**
 	 * Releases all lock that were set by us
 	 *
-	 * @return void
+	 * @return boolean Was the unlock of all locks successful?
 	 */
-	public function releaseAll() {
+	public function releaseAll()
+	{
+		$return = true;
+
 		foreach ($this->acquiredLocks as $acquiredLock => $hasLock) {
-			$this->releaseLock($acquiredLock);
+			if (!$this->releaseLock($acquiredLock)) {
+				$return = false;
+			}
 		}
+
+		return $return;
 	}
 }

src/Core/Lock/CacheLockDriver.php

@@ -65,12 +65,16 @@ class CacheLockDriver extends AbstractLockDriver
 	{
 		$cachekey = self::getLockKey($key);
 
+		$return = false;
+
 		if ($override) {
-			$this->cache->delete($cachekey);
+			$return = $this->cache->delete($cachekey);
 		} else {
-			$this->cache->compareDelete($cachekey, getmypid());
+			$return = $this->cache->compareDelete($cachekey, getmypid());
 		}
 		$this->markRelease($key);
+
+		return $return;
 	}
 
 	/**

src/Core/Lock/DatabaseLockDriver.php

@@ -76,11 +76,11 @@ class DatabaseLockDriver extends AbstractLockDriver
 			$where = ['name' => $key, 'pid' => $this->pid];
 		}
 
-		DBA::delete('locks', $where);
+		$return = DBA::delete('locks', $where);
 
 		$this->markRelease($key);
 
-		return;
+		return $return;
 	}
 
 	/**
@@ -88,9 +88,11 @@ class DatabaseLockDriver extends AbstractLockDriver
 	 */
 	public function releaseAll()
 	{
-		DBA::delete('locks', ['pid' => $this->pid]);
+		$return = DBA::delete('locks', ['pid' => $this->pid]);
 
 		$this->acquiredLocks = [];
+
+		return $return;
 	}
 
 	/**

src/Core/Lock/ILockDriver.php

@@ -36,14 +36,14 @@ interface ILockDriver
 	 * @param string $key      The Name of the lock
 	 * @param bool   $override Overrides the lock to get released
 	 *
-	 * @return void
+	 * @return boolean Was the unlock successful?
 	 */
 	public function releaseLock($key, $override = false);
 
 	/**
 	 * Releases all lock that were set by us
 	 *
-	 * @return void
+	 * @return boolean Was the unlock of all locks successful?
 	 */
 	public function releaseAll();
 }

tests/src/Core/Lock/DatabaseLockDriverTest.php

@@ -104,4 +104,15 @@ class DatabaseLockDriverTest extends LockTest
 
 		parent::testReleaseAfterUnlock();
 	}
+
+	public function testReleaseWitTTL()
+	{
+		$this->mockIsLocked('test', false, $this->startTime, 1);
+		$this->mockAcquireLock('test', 10, false, $this->pid, false, $this->startTime, 1);
+		$this->mockIsLocked('test', true, $this->startTime, 1);
+		$this->mockReleaseLock('test', $this->pid, 1);
+		$this->mockIsLocked('test', false, $this->startTime, 1);
+
+		parent::testReleaseWitTTL();
+	}
 }

tests/src/Core/Lock/LockTest.php

@@ -87,7 +87,7 @@ abstract class LockTest extends MockedTest
 		$this->assertTrue($this->instance->isLocked('bar'));
 		$this->assertTrue($this->instance->isLocked('nice'));
 
-		$this->instance->releaseAll();
+		$this->assertTrue($this->instance->releaseAll());
 
 		$this->assertFalse($this->instance->isLocked('foo'));
 		$this->assertFalse($this->instance->isLocked('bar'));
@@ -105,18 +105,30 @@ abstract class LockTest extends MockedTest
 		$this->assertTrue($this->instance->acquireLock('bar', 1));
 		$this->assertTrue($this->instance->acquireLock('nice', 1));
 
-		$this->instance->releaseLock('foo');
+		$this->assertTrue($this->instance->releaseLock('foo'));
 
 		$this->assertFalse($this->instance->isLocked('foo'));
 		$this->assertTrue($this->instance->isLocked('bar'));
 		$this->assertTrue($this->instance->isLocked('nice'));
 
-		$this->instance->releaseAll();
+		$this->assertTrue($this->instance->releaseAll());
 
 		$this->assertFalse($this->instance->isLocked('bar'));
 		$this->assertFalse($this->instance->isLocked('nice'));
 	}
 
+	/**
+	 * @small
+	 */
+	public function testReleaseWitTTL()
+	{
+		$this->assertFalse($this->instance->isLocked('test'));
+		$this->assertTrue($this->instance->acquireLock('test', 1, 10));
+		$this->assertTrue($this->instance->isLocked('test'));
+		$this->assertTrue($this->instance->releaseLock('test'));
+		$this->assertFalse($this->instance->isLocked('test'));
+	}
+
 	/**
 	 * @medium
 	 */