From f105e527149eab4565000240ff158faf913ed483 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20S=CC=8Ctekl?= <martin.stekl@gmail.com>
Date: Thu, 26 Mar 2015 10:05:43 +0100
Subject: [PATCH 1/2] Used blocking operation blPop for locking

---
 src/Kdyby/Redis/ExclusiveLock.php             | 71 +++++++++----------
 .../KdybyTests/Redis/RedisSessionHandler.phpt | 18 ++---
 2 files changed, 43 insertions(+), 46 deletions(-)

diff --git a/src/Kdyby/Redis/ExclusiveLock.php b/src/Kdyby/Redis/ExclusiveLock.php
index c0705d0..7c465c3 100644
--- a/src/Kdyby/Redis/ExclusiveLock.php
+++ b/src/Kdyby/Redis/ExclusiveLock.php
@@ -74,7 +74,7 @@ public function setClient(RedisClient $client)
 	 */
 	public function calculateTimeout()
 	{
-		return time() + abs((int)$this->duration) + 1;
+		return time() + abs((int) $this->duration) + 1;
 	}
 
 
@@ -92,36 +92,28 @@ public function acquireLock($key)
 			return $this->increaseLockTimeout($key);
 		}
 
-		$start = microtime(TRUE);
-
 		$lockKey = $this->formatLock($key);
-		$maxAttempts = 10;
-		do {
-			$sleepTime = 5000;
-			do {
-				if ($this->client->setNX($lockKey, $timeout = $this->calculateTimeout())) {
-					$this->keys[$key] = $timeout;
-					return TRUE;
-				}
-
-				if ($this->acquireTimeout !== FALSE && (microtime(TRUE) - $start) >= $this->acquireTimeout) {
-					throw LockException::acquireTimeout();
-				}
-
-				$lockExpiration = $this->client->get($lockKey);
-				$sleepTime += 2500;
-
-			} while (empty($lockExpiration) || ($lockExpiration >= time() && !usleep($sleepTime)));
-
-			$oldExpiration = $this->client->getSet($lockKey, $timeout = $this->calculateTimeout());
-			if ($oldExpiration === $lockExpiration) {
-				$this->keys[$key] = $timeout;
-				return TRUE;
-			}
+		if ($this->client->sAdd($lockKey . ':control', $lockKey) > 0) {
+			$this->client->del($lockKey);
+			$this->client->rPush($lockKey, 1);
+		}
+		$acquireTimeout = abs((int) ($this->acquireTimeout !== FALSE ? $this->acquireTimeout : $this->duration));
+		try {
+			$result = $this->client->blPop($lockKey, $acquireTimeout);
+		} catch (Kdyby\Redis\RedisClientException $e) {
+			LockException::highConcurrency();
+		}
+		if (empty($result)) {
+			throw LockException::acquireTimeout();
+		}
 
-		} while (--$maxAttempts > 0);
+		$timeout = $this->calculateTimeout();
+		$this->client->set($lockKey . ':timeout', $timeout);
+		$this->client->expireAt($lockKey . ':timeout', $timeout);
+		$this->client->expireAt($lockKey . ':control', $timeout);
+		$this->keys[$key] = $timeout;
 
-		throw LockException::highConcurrency();
+		return TRUE;
 	}
 
 
@@ -135,14 +127,13 @@ public function release($key)
 			return FALSE;
 		}
 
-		if ($this->keys[$key] <= time()) {
-			unset($this->keys[$key]);
-			return FALSE;
-		}
-
-		$this->client->del($this->formatLock($key));
+		$timeout = $this->keys[$key];
+		$lockKey = $this->formatLock($key);
+		$this->client->del($lockKey . ':timeout');
+		$this->client->rPush($lockKey, 1);
 		unset($this->keys[$key]);
-		return TRUE;
+
+		return $timeout > time();
 	}
 
 
@@ -161,11 +152,17 @@ public function increaseLockTimeout($key)
 			throw LockException::durabilityTimedOut();
 		}
 
-		$oldTimeout = $this->client->getSet($this->formatLock($key), $timeout = $this->calculateTimeout());
-		if ((int)$oldTimeout !== (int)$this->keys[$key]) {
+		$lockKey = $this->formatLock($key);
+		$timeout = $this->calculateTimeout();
+		$oldTimeout = $this->client->getSet($lockKey . ':timeout', $timeout);
+		if ((int) $oldTimeout !== (int) $this->keys[$key]) {
 			throw LockException::invalidDuration();
 		}
+
+		$this->client->expireAt($lockKey . ':timeout', $timeout);
+		$this->client->expireAt($lockKey . ':control', $timeout);
 		$this->keys[$key] = $timeout;
+
 		return TRUE;
 	}
 
diff --git a/tests/KdybyTests/Redis/RedisSessionHandler.phpt b/tests/KdybyTests/Redis/RedisSessionHandler.phpt
index 2ae5f58..cd0706e 100644
--- a/tests/KdybyTests/Redis/RedisSessionHandler.phpt
+++ b/tests/KdybyTests/Redis/RedisSessionHandler.phpt
@@ -73,7 +73,7 @@ class SessionHandlerTest extends AbstractRedisTestCase
 
 		$handler->close(); // unlock
 
-		Assert::count(1, $this->client->keys('Nette.Session:*'));
+		Assert::count(3, $this->client->keys('Nette.Session:*'));
 	}
 
 
@@ -118,7 +118,7 @@ class SessionHandlerTest extends AbstractRedisTestCase
 			array('close' => array()),
 		), $handler->methods);
 
-		Assert::count(1, $this->client->keys('Nette.Session:*'));
+		Assert::count(3, $this->client->keys('Nette.Session:*'));
 	}
 
 
@@ -178,7 +178,7 @@ class SessionHandlerTest extends AbstractRedisTestCase
 			array('close' => array()),
 		), $handler->methods);
 
-		Assert::count(1, $this->client->keys('Nette.Session:*'));
+		Assert::count(5, $this->client->keys('Nette.Session:*'));
 	}
 
 
@@ -206,7 +206,7 @@ class SessionHandlerTest extends AbstractRedisTestCase
 			Assert::same(1, $counter->visits);
 		}, 'Kdyby\Redis\SessionHandlerException', sprintf('Cannot work with non-locked session id %s: Lock couldn\'t be acquired. The locking mechanism is giving up. You should kill the request.', $sessionId));
 
-		Assert::count(1, $this->client->keys('Nette.Session:*'));
+		Assert::count(2, $this->client->keys('Nette.Session:*'));
 	}
 
 
@@ -265,18 +265,18 @@ class SessionHandlerTest extends AbstractRedisTestCase
 			$session->close(); // explicit close with unlock
 		}, 100, 30); // silence, I kill you!
 
-		self::assertRange(30, 40, $result[Tester\Runner\Runner::PASSED]);
+		self::assertRange(30, 45, $result[Tester\Runner\Runner::PASSED]);
 
-			// hard unlock
-		$client->del('Nette.Session:' . $sessionId . ':lock');
+		// hard unlock
+		$client->rPush('Nette.Session:' . $sessionId . ':lock', 1);
 
 		// open session for visits verify, for second time
 		$counter = $session->getSection('counter');
-		self::assertRange(30, 40, $counter->visits);
+		self::assertRange(30, 45, $counter->visits);
 
 		$session->close(); // unlocking drops the key
 
-		Assert::count(1, $this->client->keys('Nette.Session:*'));
+		Assert::count(3, $this->client->keys('Nette.Session:*'));
 	}
 
 

From 18de352287b95510f905690ff63e1b78c1f32732 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20S=CC=8Ctekl?= <martin.stekl@gmail.com>
Date: Thu, 26 Mar 2015 21:59:30 +0100
Subject: [PATCH 2/2] Fixed possibly too short acquireTimeout

---
 src/Kdyby/Redis/ExclusiveLock.php         | 16 ++++++++++----
 tests/KdybyTests/Redis/ExclusiveLock.phpt | 27 +++++++++++++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/src/Kdyby/Redis/ExclusiveLock.php b/src/Kdyby/Redis/ExclusiveLock.php
index 7c465c3..8c5f2bf 100644
--- a/src/Kdyby/Redis/ExclusiveLock.php
+++ b/src/Kdyby/Redis/ExclusiveLock.php
@@ -74,7 +74,7 @@ public function setClient(RedisClient $client)
 	 */
 	public function calculateTimeout()
 	{
-		return time() + abs((int) $this->duration) + 1;
+		return time() + $this->calculateRelativeTimeout() + 1;
 	}
 
 
@@ -97,9 +97,8 @@ public function acquireLock($key)
 			$this->client->del($lockKey);
 			$this->client->rPush($lockKey, 1);
 		}
-		$acquireTimeout = abs((int) ($this->acquireTimeout !== FALSE ? $this->acquireTimeout : $this->duration));
 		try {
-			$result = $this->client->blPop($lockKey, $acquireTimeout);
+			$result = $this->client->blPop($lockKey, $this->calculateRelativeTimeout());
 		} catch (Kdyby\Redis\RedisClientException $e) {
 			LockException::highConcurrency();
 		}
@@ -130,7 +129,9 @@ public function release($key)
 		$timeout = $this->keys[$key];
 		$lockKey = $this->formatLock($key);
 		$this->client->del($lockKey . ':timeout');
-		$this->client->rPush($lockKey, 1);
+		if ($timeout > time()) {
+			$this->client->rPush($lockKey, 1);
+		}
 		unset($this->keys[$key]);
 
 		return $timeout > time();
@@ -218,6 +219,13 @@ protected function formatLock($key)
 
 
 
+	protected function calculateRelativeTimeout()
+	{
+		return abs((int) ($this->acquireTimeout !== FALSE && $this->acquireTimeout > $this->duration ? $this->acquireTimeout : $this->duration));
+	}
+
+
+
 	public function __destruct()
 	{
 		$this->releaseAll();
diff --git a/tests/KdybyTests/Redis/ExclusiveLock.phpt b/tests/KdybyTests/Redis/ExclusiveLock.phpt
index a8d0f46..71a07dd 100644
--- a/tests/KdybyTests/Redis/ExclusiveLock.phpt
+++ b/tests/KdybyTests/Redis/ExclusiveLock.phpt
@@ -56,6 +56,33 @@ class ExclusiveLockTest extends AbstractRedisTestCase
 		Assert::true($second->acquireLock('foo:bar'));
 	}
 
+
+
+	public function testAcquireTimeoutLongerThenLockDuration()
+	{
+		$first = new ExclusiveLock($this->client);
+		$first->acquireTimeout = 4;
+		$first->duration = 1;
+		$second = new ExclusiveLock(new RedisClient());
+		$second->acquireTimeout = 4;
+		$second->duration = 1;
+
+		Assert::true($first->acquireLock('foo:bar'));
+		sleep(2);
+
+		Assert::exception(function () use ($second) {
+			$second->acquireLock('foo:bar');
+		}, 'Kdyby\Redis\LockException', 'Lock couldn\'t be acquired. The locking mechanism is giving up. You should kill the request.');
+		sleep(2);
+
+		Assert::true($second->acquireLock('foo:bar'));
+
+		Assert::false($first->release('foo:bar'));
+		Assert::true($second->release('foo:bar'));
+
+		Assert::same(1, $this->client->lLen('foo:bar:lock'));
+	}
+
 }
 
 \run(new ExclusiveLockTest());