From 3c943b94edc6ca3d63a58de52639b0bf4f269caf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Pineau?= Date: Fri, 28 Aug 2020 15:57:12 +0200 Subject: [PATCH] [Semaphore] Fixed some bugs --- src/Symfony/Component/Semaphore/README.md | 4 ++ .../Component/Semaphore/Store/RedisStore.php | 10 ++++- .../Resources/redis_put_off_expiration.lua | 10 +++-- .../Semaphore/Store/Resources/redis_save.lua | 8 ++-- .../Tests/Store/AbstractStoreTest.php | 42 +++++++++++++++---- .../Semaphore/Tests/Store/RedisStoreTest.php | 5 +++ 6 files changed, 60 insertions(+), 19 deletions(-) diff --git a/src/Symfony/Component/Semaphore/README.md b/src/Symfony/Component/Semaphore/README.md index 75e707f7f0..4a56641ae2 100644 --- a/src/Symfony/Component/Semaphore/README.md +++ b/src/Symfony/Component/Semaphore/README.md @@ -1,6 +1,10 @@ Semaphore Component =================== +The Semaphore Component manages +[semaphores](https://en.wikipedia.org/wiki/Semaphore_(programming)), a mechanism +to provide exclusive access to a shared resource. + **This Component is experimental**. [Experimental features](https://symfony.com/doc/current/contributing/code/experimental.html) are not covered by Symfony's diff --git a/src/Symfony/Component/Semaphore/Store/RedisStore.php b/src/Symfony/Component/Semaphore/Store/RedisStore.php index a1f41a9d64..0aae297715 100644 --- a/src/Symfony/Component/Semaphore/Store/RedisStore.php +++ b/src/Symfony/Component/Semaphore/Store/RedisStore.php @@ -78,9 +78,17 @@ class RedisStore implements PersistingStoreInterface $script = file_get_contents(__DIR__.'/Resources/redis_put_off_expiration.lua'); - if ($this->evaluate($script, sprintf('{%s}', $key), [time() + $ttlInSecond, $this->getUniqueToken($key)])) { + $ret = $this->evaluate($script, sprintf('{%s}', $key), [time() + $ttlInSecond, $this->getUniqueToken($key)]); + + // Occurs when redis has been reset + if (false === $ret) { throw new SemaphoreExpiredException($key, 'the script returns false'); } + + // Occurs when redis has added an item in the set + if (0 < $ret) { + throw new SemaphoreExpiredException($key, 'the script returns a positive number'); + } } /** diff --git a/src/Symfony/Component/Semaphore/Store/Resources/redis_put_off_expiration.lua b/src/Symfony/Component/Semaphore/Store/Resources/redis_put_off_expiration.lua index 0c4ff3fb8a..b260a2e451 100644 --- a/src/Symfony/Component/Semaphore/Store/Resources/redis_put_off_expiration.lua +++ b/src/Symfony/Component/Semaphore/Store/Resources/redis_put_off_expiration.lua @@ -9,10 +9,12 @@ if added == 1 then end -- Extend the TTL -local curentTtl = redis.call("TTL", weightKey) -if curentTtl < now + ttlInSecond then - redis.call("EXPIRE", weightKey, curentTtl + 10) - redis.call("EXPIRE", timeKey, curentTtl + 10) +local maxExpiration = redis.call("ZREVRANGE", timeKey, 0, 0, "WITHSCORES")[2] +if nil == maxExpiration then + return 1 end +redis.call("EXPIREAT", weightKey, maxExpiration + 10) +redis.call("EXPIREAT", timeKey, maxExpiration + 10) + return added diff --git a/src/Symfony/Component/Semaphore/Store/Resources/redis_save.lua b/src/Symfony/Component/Semaphore/Store/Resources/redis_save.lua index 50942b53c9..c0cbac5f5a 100644 --- a/src/Symfony/Component/Semaphore/Store/Resources/redis_save.lua +++ b/src/Symfony/Component/Semaphore/Store/Resources/redis_save.lua @@ -34,10 +34,8 @@ redis.call("ZADD", timeKey, now + ttlInSecond, identifier) redis.call("ZADD", weightKey, weight, identifier) -- Extend the TTL -local curentTtl = redis.call("TTL", weightKey) -if curentTtl < now + ttlInSecond then - redis.call("EXPIRE", weightKey, curentTtl + 10) - redis.call("EXPIRE", timeKey, curentTtl + 10) -end +local maxExpiration = redis.call("ZREVRANGE", timeKey, 0, 0, "WITHSCORES")[2] +redis.call("EXPIREAT", weightKey, maxExpiration + 10) +redis.call("EXPIREAT", timeKey, maxExpiration + 10) return true diff --git a/src/Symfony/Component/Semaphore/Tests/Store/AbstractStoreTest.php b/src/Symfony/Component/Semaphore/Tests/Store/AbstractStoreTest.php index 8715d4d11c..999b0ea8c8 100644 --- a/src/Symfony/Component/Semaphore/Tests/Store/AbstractStoreTest.php +++ b/src/Symfony/Component/Semaphore/Tests/Store/AbstractStoreTest.php @@ -13,6 +13,7 @@ namespace Symfony\Component\Semaphore\Tests\Store; use PHPUnit\Framework\TestCase; use Symfony\Component\Semaphore\Exception\SemaphoreAcquiringException; +use Symfony\Component\Semaphore\Exception\SemaphoreExpiredException; use Symfony\Component\Semaphore\Key; use Symfony\Component\Semaphore\PersistingStoreInterface; @@ -28,7 +29,7 @@ abstract class AbstractStoreTest extends TestCase { $store = $this->getStore(); - $key = new Key('key', 1); + $key = new Key(__METHOD__, 1); $this->assertFalse($store->exists($key)); $store->save($key, 10); @@ -41,8 +42,8 @@ abstract class AbstractStoreTest extends TestCase { $store = $this->getStore(); - $key1 = new Key('key1', 1); - $key2 = new Key('key2', 1); + $key1 = new Key(__METHOD__.'1', 1); + $key2 = new Key(__METHOD__.'2', 1); $store->save($key1, 10); $this->assertTrue($store->exists($key1)); @@ -65,7 +66,7 @@ abstract class AbstractStoreTest extends TestCase { $store = $this->getStore(); - $resource = 'resource'; + $resource = __METHOD__; $key1 = new Key($resource, 1); $key2 = new Key($resource, 1); @@ -100,7 +101,7 @@ abstract class AbstractStoreTest extends TestCase { $store = $this->getStore(); - $resource = 'resource'; + $resource = __METHOD__; $key1 = new Key($resource, 2); $key2 = new Key($resource, 2); $key3 = new Key($resource, 2); @@ -144,7 +145,7 @@ abstract class AbstractStoreTest extends TestCase { $store = $this->getStore(); - $resource = 'resource'; + $resource = __METHOD__; $key1 = new Key($resource, 4, 2); $key2 = new Key($resource, 4, 2); $key3 = new Key($resource, 4, 2); @@ -184,17 +185,40 @@ abstract class AbstractStoreTest extends TestCase $store->delete($key3); } + public function testPutOffExpiration() + { + $store = $this->getStore(); + $key = new Key(__METHOD__, 4, 2); + $store->save($key, 20); + + $store->putOffExpiration($key, 20); + + // just asserts it doesn't throw an exception + $this->addToAssertionCount(1); + } + + public function testPutOffExpirationWhenSaveHasNotBeenCalled() + { + // This test simulate the key has expired since it does not exist + $store = $this->getStore(); + $key1 = new Key(__METHOD__, 4, 2); + + $this->expectException(SemaphoreExpiredException::class); + $this->expectExceptionMessage('The semaphore "Symfony\Component\Semaphore\Tests\Store\AbstractStoreTest::testPutOffExpirationWhenSaveHasNotBeenCalled" has expired: the script returns a positive number.'); + + $store->putOffExpiration($key1, 20); + } + public function testSaveTwice() { $store = $this->getStore(); - $resource = 'resource'; - $key = new Key($resource, 1); + $key = new Key(__METHOD__, 1); $store->save($key, 10); $store->save($key, 10); - // just asserts it don't throw an exception + // just asserts it doesn't throw an exception $this->addToAssertionCount(1); $store->delete($key); diff --git a/src/Symfony/Component/Semaphore/Tests/Store/RedisStoreTest.php b/src/Symfony/Component/Semaphore/Tests/Store/RedisStoreTest.php index 5b05f38355..ac35eef424 100644 --- a/src/Symfony/Component/Semaphore/Tests/Store/RedisStoreTest.php +++ b/src/Symfony/Component/Semaphore/Tests/Store/RedisStoreTest.php @@ -18,6 +18,11 @@ namespace Symfony\Component\Semaphore\Tests\Store; */ class RedisStoreTest extends AbstractRedisStoreTest { + protected function setUp(): void + { + $this->getRedisConnection()->flushDB(); + } + public static function setUpBeforeClass(): void { try {