From 7b171d0531c59b5a15fd1d07b1c379f643de6dc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sat, 7 Dec 2024 22:49:37 +0100 Subject: [PATCH 01/24] introduce AbstractSpinlockExpireMutex --- .../ExecutionOutsideLockException.php | 3 +- src/Mutex/AbstractLockMutex.php | 2 - src/Mutex/AbstractRedlockMutex.php | 35 ++++---- src/Mutex/AbstractSpinlockExpireMutex.php | 81 +++++++++++++++++++ src/Mutex/AbstractSpinlockMutex.php | 18 ++--- src/Mutex/MemcachedMutex.php | 23 ++++-- tests/Mutex/AbstractRedlockMutexTest.php | 12 +-- .../Mutex/AbstractSpinlockExpireMutexTest.php | 65 +++++++++++++++ tests/Mutex/AbstractSpinlockMutexTest.php | 26 +----- tests/Mutex/MutexTest.php | 26 +++--- 10 files changed, 207 insertions(+), 84 deletions(-) create mode 100644 src/Mutex/AbstractSpinlockExpireMutex.php create mode 100644 tests/Mutex/AbstractSpinlockExpireMutexTest.php diff --git a/src/Exception/ExecutionOutsideLockException.php b/src/Exception/ExecutionOutsideLockException.php index e4bd7bcd..c8bb70b7 100644 --- a/src/Exception/ExecutionOutsideLockException.php +++ b/src/Exception/ExecutionOutsideLockException.php @@ -4,6 +4,7 @@ namespace Malkusch\Lock\Exception; +use Malkusch\Lock\Mutex\AbstractSpinlockMutex; use Malkusch\Lock\Util\LockUtil; /** @@ -14,7 +15,7 @@ * * Should only be used in contexts where the lock is being released. * - * @see \Malkusch\Lock\Mutex\AbstractSpinlockMutex::unlock() + * @see AbstractSpinlockMutex::unlock() */ class ExecutionOutsideLockException extends LockReleaseException { diff --git a/src/Mutex/AbstractLockMutex.php b/src/Mutex/AbstractLockMutex.php index 745d46c0..e663d2c1 100644 --- a/src/Mutex/AbstractLockMutex.php +++ b/src/Mutex/AbstractLockMutex.php @@ -8,8 +8,6 @@ use Malkusch\Lock\Exception\LockReleaseException; /** - * Locking mutex. - * * @internal */ abstract class AbstractLockMutex extends AbstractMutex diff --git a/src/Mutex/AbstractRedlockMutex.php b/src/Mutex/AbstractRedlockMutex.php index d3b7bdde..e65c4a71 100644 --- a/src/Mutex/AbstractRedlockMutex.php +++ b/src/Mutex/AbstractRedlockMutex.php @@ -18,44 +18,43 @@ * * @see http://redis.io/topics/distlock */ -abstract class AbstractRedlockMutex extends AbstractSpinlockMutex implements LoggerAwareInterface +abstract class AbstractRedlockMutex extends AbstractSpinlockExpireMutex implements LoggerAwareInterface { use LoggerAwareTrait; /** @var array */ private array $clients; - private string $token; - /** - * The Redis APIs needs to be connected. I.e. Redis::connect() was + * The Redis instance needs to be connected. I.e. Redis::connect() was * called already. * * @param array $clients * @param float $acquireTimeout In seconds + * @param float $expireTimeout In seconds */ - public function __construct(array $clients, string $name, float $acquireTimeout = 3) + public function __construct(array $clients, string $name, float $acquireTimeout = 3, float $expireTimeout = \PHP_INT_MAX) { - parent::__construct($name, $acquireTimeout); + parent::__construct($name, $acquireTimeout, $expireTimeout); $this->clients = $clients; $this->logger = new NullLogger(); } #[\Override] - protected function acquire(string $key, float $expire): bool + protected function acquireWithToken(string $key, float $expireTimeout) { // 1. This differs from the specification to avoid an overflow on 32-Bit systems. - $time = microtime(true); + $startTs = microtime(true); // 2. $acquired = 0; $errored = 0; - $this->token = LockUtil::getInstance()->makeRandomToken(); + $token = LockUtil::getInstance()->makeRandomToken(); $exception = null; foreach ($this->clients as $index => $client) { try { - if ($this->add($client, $key, $this->token, $expire)) { + if ($this->add($client, $key, $token, $expireTimeout)) { ++$acquired; } } catch (LockAcquireException $exception) { @@ -63,7 +62,7 @@ protected function acquire(string $key, float $expire): bool $context = [ 'key' => $key, 'index' => $index, - 'token' => $this->token, + 'token' => $token, 'exception' => $exception, ]; $this->logger->warning('Could not set {key} = {token} at server #{index}', $context); @@ -73,16 +72,16 @@ protected function acquire(string $key, float $expire): bool } // 3. - $elapsedTime = microtime(true) - $time; - $isAcquired = $this->isMajority($acquired) && $elapsedTime <= $expire; + $elapsedTime = microtime(true) - $startTs; + $isAcquired = $this->isMajority($acquired) && $elapsedTime <= $expireTimeout; if ($isAcquired) { // 4. - return true; + return $token; } // 5. - $this->release($key); + $this->releaseWithToken($key, $token); // In addition to RedLock it's an exception if too many servers fail. if (!$this->isMajority(count($this->clients) - $errored)) { @@ -99,7 +98,7 @@ protected function acquire(string $key, float $expire): bool } #[\Override] - protected function release(string $key): bool + protected function releaseWithToken(string $key, string $token): bool { /* * All Redis commands must be analyzed before execution to determine which keys the command will operate on. In @@ -117,7 +116,7 @@ protected function release(string $key): bool $released = 0; foreach ($this->clients as $index => $client) { try { - if ($this->evalScript($client, $script, [$key], [$this->token])) { + if ($this->evalScript($client, $script, [$key], [$token])) { ++$released; } } catch (LockReleaseException $e) { @@ -125,7 +124,7 @@ protected function release(string $key): bool $context = [ 'key' => $key, 'index' => $index, - 'token' => $this->token, + 'token' => $token, 'exception' => $e, ]; $this->logger->warning('Could not unset {key} = {token} at server #{index}', $context); diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockExpireMutex.php new file mode 100644 index 00000000..92f36a86 --- /dev/null +++ b/src/Mutex/AbstractSpinlockExpireMutex.php @@ -0,0 +1,81 @@ +expireTimeout = $expireTimeout; + } + + #[\Override] + final protected function acquire(string $key): bool + { + /* + * The expiration timeout for the lock is increased by one second + * to ensure that we delete only our keys. This will prevent the + * case that this key expires before the timeout, and another process + * acquires successfully the same key which would then be deleted + * by this process. + */ + $res = $this->acquireWithToken($key, $this->expireTimeout + 1); + + if ($res === false) { + return false; + } + + \assert(is_string($res) && strlen($res) > 1); // @phpstan-ignore function.alreadyNarrowedType + + $this->token = $res; + + return true; + } + + #[\Override] + final protected function release(string $key): bool + { + try { + return $this->releaseWithToken($key, $this->token); + } finally { + $this->token = null; + } + } + + /** + * Same as self::acquire() but with expire timeout and token. + * + * @param non-falsy-string $key + * @param float $expireTimeout In seconds + * + * @return non-falsy-string|false + */ + abstract protected function acquireWithToken(string $key, float $expireTimeout); + + /** + * Same as self::release() but with expire timeout and token. + * + * @param non-falsy-string $key + * @param non-falsy-string $token + */ + abstract protected function releaseWithToken(string $key, string $token): bool; +} diff --git a/src/Mutex/AbstractSpinlockMutex.php b/src/Mutex/AbstractSpinlockMutex.php index 33583b6c..42050ae2 100644 --- a/src/Mutex/AbstractSpinlockMutex.php +++ b/src/Mutex/AbstractSpinlockMutex.php @@ -12,11 +12,10 @@ /** * Spinlock implementation. - * - * @internal */ abstract class AbstractSpinlockMutex extends AbstractLockMutex { + /** @var non-falsy-string */ private string $key; /** In seconds */ @@ -42,14 +41,7 @@ protected function lock(): void $loop->execute(function () use ($loop): void { $this->acquiredTs = microtime(true); - /* - * The expiration timeout for the lock is increased by one second - * to ensure that we delete only our keys. This will prevent the - * case that this key expires before the timeout, and another process - * acquires successfully the same key which would then be deleted - * by this process. - */ - if ($this->acquire($this->key, $this->acquireTimeout + 1)) { + if ($this->acquire($this->key)) { $loop->end(); } }, $this->acquireTimeout); @@ -75,17 +67,19 @@ protected function unlock(): void /** * Try to acquire a lock. * - * @param float $expire In seconds + * @param non-falsy-string $key * * @return bool True if the lock was acquired * * @throws LockAcquireException An unexpected error happened */ - abstract protected function acquire(string $key, float $expire): bool; + abstract protected function acquire(string $key): bool; /** * Try to release a lock. * + * @param non-falsy-string $key + * * @return bool True if the lock was released */ abstract protected function release(string $key): bool; diff --git a/src/Mutex/MemcachedMutex.php b/src/Mutex/MemcachedMutex.php index 6eee2132..9dda8136 100644 --- a/src/Mutex/MemcachedMutex.php +++ b/src/Mutex/MemcachedMutex.php @@ -9,36 +9,43 @@ /** * Memcached based spinlock implementation. */ -class MemcachedMutex extends AbstractSpinlockMutex +class MemcachedMutex extends AbstractSpinlockExpireMutex { private \Memcached $memcached; /** - * The Memcached API needs to have at least one server in its pool. I.e. + * The Memcached instance needs to have at least one server in its pool. I.e. * it has to be added with Memcached::addServer(). * * @param float $acquireTimeout In seconds + * @param float $expireTimeout In seconds */ - public function __construct(string $name, \Memcached $memcached, float $acquireTimeout = 3) + public function __construct(string $name, \Memcached $memcached, float $acquireTimeout = 3, float $expireTimeout = \PHP_INT_MAX) { - parent::__construct($name, $acquireTimeout); + parent::__construct($name, $acquireTimeout, $expireTimeout); $this->memcached = $memcached; } #[\Override] - protected function acquire(string $key, float $expire): bool + protected function acquireWithToken(string $key, float $expireTimeout) { // memcached supports only integer expire // https://github.com/memcached/memcached/wiki/Commands#standard-protocol - $expireInt = LockUtil::getInstance()->castFloatToInt(ceil($expire)); + $expireTimeoutInt = LockUtil::getInstance()->castFloatToInt(ceil($expireTimeout)); - return $this->memcached->add($key, true, $expireInt); + $token = LockUtil::getInstance()->makeRandomToken(); + + return $this->memcached->add($key, $token, $expireTimeoutInt) + ? $token + : false; } #[\Override] - protected function release(string $key): bool + protected function releaseWithToken(string $key, string $token): bool { + // TODO atomic delete only when the remove value matches token + return $this->memcached->delete($key); } } diff --git a/tests/Mutex/AbstractRedlockMutexTest.php b/tests/Mutex/AbstractRedlockMutexTest.php index 0c3e6628..301bc1f6 100644 --- a/tests/Mutex/AbstractRedlockMutexTest.php +++ b/tests/Mutex/AbstractRedlockMutexTest.php @@ -9,6 +9,7 @@ use Malkusch\Lock\Exception\LockReleaseException; use Malkusch\Lock\Exception\MutexException; use Malkusch\Lock\Mutex\AbstractRedlockMutex; +use Malkusch\Lock\Util\LockUtil; use phpmock\environment\SleepEnvironmentBuilder; use phpmock\MockEnabledException; use phpmock\phpunit\PHPMock; @@ -44,7 +45,7 @@ protected function setUp(): void * * @return AbstractRedlockMutex&MockObject */ - private function createRedlockMutexMock(int $count, float $timeout = 1): AbstractRedlockMutex + private function createRedlockMutexMock(int $count, float $acquireTimeout = 1, float $expireTimeout = \PHP_INT_MAX): AbstractRedlockMutex { $clients = array_map( static fn ($i) => new class($i) { @@ -59,7 +60,7 @@ public function __construct(int $i) ); return $this->getMockBuilder(AbstractRedlockMutex::class) - ->setConstructorArgs([$clients, 'test', $timeout]) + ->setConstructorArgs([$clients, 'test', $acquireTimeout, $expireTimeout]) ->onlyMethods(['add', 'evalScript']) ->getMock(); } @@ -178,13 +179,8 @@ static function () use (&$i, $available): bool { #[DataProvider('provideAcquireTimeoutsCases')] public function testAcquireTimeouts(int $count, float $timeout, float $delay): void { - $timeoutStr = (string) round($timeout, 6); - if (strpos($timeoutStr, '.') === false) { - $timeoutStr .= '.0'; - } - $this->expectException(LockAcquireTimeoutException::class); - $this->expectExceptionMessage('Lock acquire timeout of ' . $timeoutStr . ' seconds has been exceeded'); + $this->expectExceptionMessage('Lock acquire timeout of ' . LockUtil::getInstance()->formatTimeout($timeout) . ' seconds has been exceeded'); $mutex = $this->createRedlockMutexMock($count, $timeout); diff --git a/tests/Mutex/AbstractSpinlockExpireMutexTest.php b/tests/Mutex/AbstractSpinlockExpireMutexTest.php new file mode 100644 index 00000000..a69818e3 --- /dev/null +++ b/tests/Mutex/AbstractSpinlockExpireMutexTest.php @@ -0,0 +1,65 @@ +addNamespace(__NAMESPACE__); + $sleepBuilder->addNamespace('Malkusch\Lock\Mutex'); + $sleepBuilder->addNamespace('Malkusch\Lock\Util'); + $sleep = $sleepBuilder->build(); + try { + $sleep->enable(); + $this->registerForTearDown($sleep); + } catch (MockEnabledException $e) { + // workaround for burn testing + \assert($e->getMessage() === 'microtime is already enabled. Call disable() on the existing mock.'); + } + } + + /** + * @return AbstractSpinlockExpireMutex&MockObject + */ + private function createSpinlockExpireMutexMock(float $acquireTimeout = 3, float $expireTimeout = \PHP_INT_MAX): AbstractSpinlockExpireMutex + { + return $this->getMockBuilder(AbstractSpinlockExpireMutex::class) + ->setConstructorArgs(['test', $acquireTimeout, $expireTimeout]) + ->onlyMethods(['acquireWithToken', 'releaseWithToken']) + ->getMock(); + } + + /** + * Tests executing exactly until the timeout will leave the key one more second. + */ + public function testExecuteTimeoutLeavesOneSecondForKeyToExpire(): void + { + $mutex = $this->createSpinlockExpireMutexMock(0.2, 0.3); + $mutex->expects(self::once()) + ->method('acquireWithToken') + ->with(self::anything(), 1.3) + ->willReturn('xx'); + + $mutex->expects(self::once())->method('releaseWithToken')->willReturn(true); + + $mutex->synchronized(static function () { + usleep(199 * 1000); + }); + } +} diff --git a/tests/Mutex/AbstractSpinlockMutexTest.php b/tests/Mutex/AbstractSpinlockMutexTest.php index c216d1e2..db973814 100644 --- a/tests/Mutex/AbstractSpinlockMutexTest.php +++ b/tests/Mutex/AbstractSpinlockMutexTest.php @@ -41,10 +41,10 @@ protected function setUp(): void /** * @return AbstractSpinlockMutex&MockObject */ - private function createSpinlockMutexMock(float $timeout = 3): AbstractSpinlockMutex + private function createSpinlockMutexMock(float $acquireTimeout = 3): AbstractSpinlockMutex { return $this->getMockBuilder(AbstractSpinlockMutex::class) - ->setConstructorArgs(['test', $timeout]) + ->setConstructorArgs(['test', $acquireTimeout]) ->onlyMethods(['acquire', 'release']) ->getMock(); } @@ -85,7 +85,7 @@ public function testAcquireTimeouts(): void } /** - * Tests executing code which exceeds the timeout fails. + * Tests executing code which exceeds the acquire timeout fails. */ public function testExecuteTooLong(): void { @@ -107,7 +107,7 @@ public function testExecuteTooLong(): void } /** - * Tests executing code which barely doesn't hit the timeout. + * Tests executing code which barely doesn't hit the acquire timeout. */ public function testExecuteBarelySucceeds(): void { @@ -133,22 +133,4 @@ public function testFailReleasingLock(): void $mutex->synchronized(static function () {}); } - - /** - * Tests executing exactly until the timeout will leave the key one more second. - */ - public function testExecuteTimeoutLeavesOneSecondForKeyToExpire(): void - { - $mutex = $this->createSpinlockMutexMock(0.2); - $mutex->expects(self::once()) - ->method('acquire') - ->with(self::anything(), 1.2) - ->willReturn(true); - - $mutex->expects(self::once())->method('release')->willReturn(true); - - $mutex->synchronized(static function () { - usleep(199 * 1000); - }); - } } diff --git a/tests/Mutex/MutexTest.php b/tests/Mutex/MutexTest.php index 45fccfb4..9ea6ee9c 100644 --- a/tests/Mutex/MutexTest.php +++ b/tests/Mutex/MutexTest.php @@ -86,10 +86,22 @@ public static function provideMutexFactoriesCases(): iterable }]; } + yield 'AbstractLockMutex' => [static function (): Mutex { + $lock = new class extends AbstractLockMutex { + #[\Override] + protected function lock(): void {} + + #[\Override] + protected function unlock(): void {} + }; + + return $lock; + }]; + yield 'AbstractSpinlockMutex' => [static function (): Mutex { $lock = new class('test') extends AbstractSpinlockMutex { #[\Override] - protected function acquire(string $key, float $expire): bool + protected function acquire(string $key): bool { return true; } @@ -104,18 +116,6 @@ protected function release(string $key): bool return $lock; }]; - yield 'AbstractLockMutex' => [static function (): Mutex { - $lock = new class extends AbstractLockMutex { - #[\Override] - protected function lock(): void {} - - #[\Override] - protected function unlock(): void {} - }; - - return $lock; - }]; - if (getenv('MEMCACHE_HOST')) { yield 'MemcachedMutex' => [static function (): Mutex { $memcached = new \Memcached(); From 2761ef1c250c11228209f08ab552cab3eb519bc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 12:08:46 +0100 Subject: [PATCH 02/24] wip --- src/Mutex/AbstractSpinlockMutex.php | 15 ------------ .../Mutex/AbstractSpinlockExpireMutexTest.php | 23 +++++++++++++++++++ tests/Mutex/AbstractSpinlockMutexTest.php | 23 ------------------- 3 files changed, 23 insertions(+), 38 deletions(-) diff --git a/src/Mutex/AbstractSpinlockMutex.php b/src/Mutex/AbstractSpinlockMutex.php index 42050ae2..36967ce3 100644 --- a/src/Mutex/AbstractSpinlockMutex.php +++ b/src/Mutex/AbstractSpinlockMutex.php @@ -4,7 +4,6 @@ namespace Malkusch\Lock\Mutex; -use Malkusch\Lock\Exception\ExecutionOutsideLockException; use Malkusch\Lock\Exception\LockAcquireException; use Malkusch\Lock\Exception\LockReleaseException; use Malkusch\Lock\Util\LockUtil; @@ -21,9 +20,6 @@ abstract class AbstractSpinlockMutex extends AbstractLockMutex /** In seconds */ private float $acquireTimeout; - /** The timestamp when the lock was acquired */ - private ?float $acquiredTs = null; - /** * @param float $acquireTimeout In seconds */ @@ -39,8 +35,6 @@ protected function lock(): void $loop = new Loop(); $loop->execute(function () use ($loop): void { - $this->acquiredTs = microtime(true); - if ($this->acquire($this->key)) { $loop->end(); } @@ -50,15 +44,6 @@ protected function lock(): void #[\Override] protected function unlock(): void { - $elapsedTime = microtime(true) - $this->acquiredTs; - if ($elapsedTime > $this->acquireTimeout) { - throw ExecutionOutsideLockException::create($elapsedTime, $this->acquireTimeout); - } - - /* - * Worst case would still be one second before the key expires. - * This guarantees that we don't delete a wrong key. - */ if (!$this->release($this->key)) { throw new LockReleaseException('Failed to release the lock'); } diff --git a/tests/Mutex/AbstractSpinlockExpireMutexTest.php b/tests/Mutex/AbstractSpinlockExpireMutexTest.php index a69818e3..1c0f3d80 100644 --- a/tests/Mutex/AbstractSpinlockExpireMutexTest.php +++ b/tests/Mutex/AbstractSpinlockExpireMutexTest.php @@ -4,6 +4,7 @@ namespace Malkusch\Lock\Tests\Mutex; +use Malkusch\Lock\Exception\ExecutionOutsideLockException; use Malkusch\Lock\Mutex\AbstractSpinlockExpireMutex; use phpmock\environment\SleepEnvironmentBuilder; use phpmock\MockEnabledException; @@ -45,6 +46,28 @@ private function createSpinlockExpireMutexMock(float $acquireTimeout = 3, float ->getMock(); } + /** + * Tests executing code which exceeds the acquire timeout fails. + */ + public function testExecuteTooLong(): void + { + $mutex = $this->createSpinlockExpireMutexMock(0.5); + $mutex->expects(self::any()) + ->method('acquireWithToken') + ->willReturn('xx'); + + $mutex->expects(self::any()) + ->method('releaseWithToken') + ->willReturn(true); + + $this->expectException(ExecutionOutsideLockException::class); + $this->expectExceptionMessageMatches('~^The code executed for 0\.5\d+ seconds\. But the timeout is 0\.5 seconds. The last 0\.0\d+ seconds were executed outside of the lock\.$~'); + + $mutex->synchronized(static function () { + usleep(501 * 1000); + }); + } + /** * Tests executing exactly until the timeout will leave the key one more second. */ diff --git a/tests/Mutex/AbstractSpinlockMutexTest.php b/tests/Mutex/AbstractSpinlockMutexTest.php index db973814..b993edde 100644 --- a/tests/Mutex/AbstractSpinlockMutexTest.php +++ b/tests/Mutex/AbstractSpinlockMutexTest.php @@ -4,7 +4,6 @@ namespace Malkusch\Lock\Tests\Mutex; -use Malkusch\Lock\Exception\ExecutionOutsideLockException; use Malkusch\Lock\Exception\LockAcquireException; use Malkusch\Lock\Exception\LockAcquireTimeoutException; use Malkusch\Lock\Exception\LockReleaseException; @@ -84,28 +83,6 @@ public function testAcquireTimeouts(): void }); } - /** - * Tests executing code which exceeds the acquire timeout fails. - */ - public function testExecuteTooLong(): void - { - $mutex = $this->createSpinlockMutexMock(0.5); - $mutex->expects(self::any()) - ->method('acquire') - ->willReturn(true); - - $mutex->expects(self::any()) - ->method('release') - ->willReturn(true); - - $this->expectException(ExecutionOutsideLockException::class); - $this->expectExceptionMessageMatches('~^The code executed for 0\.5\d+ seconds\. But the timeout is 0\.5 seconds. The last 0\.0\d+ seconds were executed outside of the lock\.$~'); - - $mutex->synchronized(static function () { - usleep(501 * 1000); - }); - } - /** * Tests executing code which barely doesn't hit the acquire timeout. */ From 3c0757067d7e71ec438b44d3e8a0f1e9792cfd23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 12:17:50 +0100 Subject: [PATCH 03/24] wip2 with todos --- src/Mutex/AbstractSpinlockExpireMutex.php | 36 ++++++++++++++++------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockExpireMutex.php index 92f36a86..43ce3f5d 100644 --- a/src/Mutex/AbstractSpinlockExpireMutex.php +++ b/src/Mutex/AbstractSpinlockExpireMutex.php @@ -4,6 +4,8 @@ namespace Malkusch\Lock\Mutex; +use Malkusch\Lock\Exception\ExecutionOutsideLockException; + /** * Spinlock implementation with expirable resource locking. * @@ -11,12 +13,14 @@ */ abstract class AbstractSpinlockExpireMutex extends AbstractSpinlockMutex { - /** @var non-falsy-string */ - private ?string $token = null; - /** In seconds */ private float $expireTimeout; + private ?float $acquireTs = null; + + /** @var non-falsy-string */ + private ?string $token = null; + /** * @param float $acquireTimeout In seconds * @param float $expireTimeout In seconds @@ -29,31 +33,41 @@ public function __construct(string $name, float $acquireTimeout = 3, float $expi } #[\Override] - final protected function acquire(string $key): bool + protected function acquire(string $key): bool { + $acquireTs = microtime(true); + /* - * The expiration timeout for the lock is increased by one second + * The expiration timeout for the lock is increased by 1 second * to ensure that we delete only our keys. This will prevent the * case that this key expires before the timeout, and another process * acquires successfully the same key which would then be deleted * by this process. + * + * TODO 1 second should no longer be added as there are two separate timeouts newly - acquire and expire */ - $res = $this->acquireWithToken($key, $this->expireTimeout + 1); + $token = $this->acquireWithToken($key, $this->expireTimeout + 1); - if ($res === false) { + if ($token === false) { return false; } - \assert(is_string($res) && strlen($res) > 1); // @phpstan-ignore function.alreadyNarrowedType - - $this->token = $res; + $this->acquireTs = $acquireTs; + $this->token = $token; return true; } #[\Override] - final protected function release(string $key): bool + protected function release(string $key): bool { + // TODO expire timeout should be checked here and token should be always tried to be released + $acquireTimeout = \Closure::bind(fn () => $this->acquireTimeout, $this, parent::class)(); + $elapsedTime = microtime(true) - $this->acquireTs; + if ($elapsedTime > $acquireTimeout) { + throw ExecutionOutsideLockException::create($elapsedTime, $acquireTimeout); + } + try { return $this->releaseWithToken($key, $this->token); } finally { From 08534a1e1d740838d8f243ba313fe9581f74f24f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 12:31:46 +0100 Subject: [PATCH 04/24] adjust RedisMutexWithPredisTest --- tests/Mutex/RedisMutexWithPredisTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/Mutex/RedisMutexWithPredisTest.php b/tests/Mutex/RedisMutexWithPredisTest.php index 32c78f5e..f6c3feb6 100644 --- a/tests/Mutex/RedisMutexWithPredisTest.php +++ b/tests/Mutex/RedisMutexWithPredisTest.php @@ -45,7 +45,7 @@ protected function setUp(): void $this->client = $this->createMock(PredisClientInterfaceWithSetAndEvalMethods::class); - $this->mutex = new RedisMutex([$this->client], 'test', 2.5); + $this->mutex = new RedisMutex([$this->client], 'test', 2.5, 3.5); $this->logger = $this->createMock(LoggerInterface::class); $this->mutex->setLogger($this->logger); @@ -58,7 +58,7 @@ public function testAddFailsToSetKey(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') ->willReturn(null); $this->logger->expects(self::never()) @@ -80,7 +80,7 @@ public function testAddErrors(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') ->willThrowException($this->createMock(PredisException::class)); $this->logger->expects(self::once()) @@ -100,7 +100,7 @@ public function testWorksNormally(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') ->willReturnSelf(); $this->client->expects(self::once()) @@ -124,7 +124,7 @@ public function testEvalScriptFails(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') ->willReturnSelf(); $this->client->expects(self::once()) From 3fcc5a143c943679a37a646a14815c4923e67382 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 12:51:10 +0100 Subject: [PATCH 05/24] fix BC for tests --- src/Mutex/AbstractSpinlockExpireMutex.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockExpireMutex.php index 43ce3f5d..4c9af909 100644 --- a/src/Mutex/AbstractSpinlockExpireMutex.php +++ b/src/Mutex/AbstractSpinlockExpireMutex.php @@ -29,6 +29,11 @@ public function __construct(string $name, float $acquireTimeout = 3, float $expi { parent::__construct($name, $acquireTimeout); + // TODO remove this BC once all tests fixed + if ($expireTimeout === (float) \PHP_INT_MAX) { + $expireTimeout = $acquireTimeout; + } + $this->expireTimeout = $expireTimeout; } From 43d4d07b722450059fa6c5fb26a23003fc93e500 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 13:47:13 +0100 Subject: [PATCH 06/24] impl AbstractSpinlockExpireMutex::release todo --- src/Mutex/AbstractSpinlockExpireMutex.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockExpireMutex.php index 4c9af909..9e2667fc 100644 --- a/src/Mutex/AbstractSpinlockExpireMutex.php +++ b/src/Mutex/AbstractSpinlockExpireMutex.php @@ -66,17 +66,18 @@ protected function acquire(string $key): bool #[\Override] protected function release(string $key): bool { - // TODO expire timeout should be checked here and token should be always tried to be released - $acquireTimeout = \Closure::bind(fn () => $this->acquireTimeout, $this, parent::class)(); - $elapsedTime = microtime(true) - $this->acquireTs; - if ($elapsedTime > $acquireTimeout) { - throw ExecutionOutsideLockException::create($elapsedTime, $acquireTimeout); - } - try { return $this->releaseWithToken($key, $this->token); } finally { - $this->token = null; + try { + $elapsedTime = microtime(true) - $this->acquireTs; + if ($elapsedTime >= $this->expireTimeout) { + throw ExecutionOutsideLockException::create($elapsedTime, $this->expireTimeout); + } + } finally { + $this->token = null; + $this->acquireTs = null; + } } } From 73ae2c378705ee9505bc08e6f7554469d356e820 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 14:43:55 +0100 Subject: [PATCH 07/24] better infinity expire timeout default --- src/Mutex/AbstractRedlockMutex.php | 2 +- src/Mutex/AbstractSpinlockExpireMutex.php | 4 ++-- src/Mutex/MemcachedMutex.php | 2 +- tests/Mutex/AbstractRedlockMutexTest.php | 2 +- tests/Mutex/AbstractSpinlockExpireMutexTest.php | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Mutex/AbstractRedlockMutex.php b/src/Mutex/AbstractRedlockMutex.php index e65c4a71..6683f323 100644 --- a/src/Mutex/AbstractRedlockMutex.php +++ b/src/Mutex/AbstractRedlockMutex.php @@ -33,7 +33,7 @@ abstract class AbstractRedlockMutex extends AbstractSpinlockExpireMutex implemen * @param float $acquireTimeout In seconds * @param float $expireTimeout In seconds */ - public function __construct(array $clients, string $name, float $acquireTimeout = 3, float $expireTimeout = \PHP_INT_MAX) + public function __construct(array $clients, string $name, float $acquireTimeout = 3, float $expireTimeout = \INF) { parent::__construct($name, $acquireTimeout, $expireTimeout); diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockExpireMutex.php index 9e2667fc..88a2d5d4 100644 --- a/src/Mutex/AbstractSpinlockExpireMutex.php +++ b/src/Mutex/AbstractSpinlockExpireMutex.php @@ -25,12 +25,12 @@ abstract class AbstractSpinlockExpireMutex extends AbstractSpinlockMutex * @param float $acquireTimeout In seconds * @param float $expireTimeout In seconds */ - public function __construct(string $name, float $acquireTimeout = 3, float $expireTimeout = \PHP_INT_MAX) + public function __construct(string $name, float $acquireTimeout = 3, float $expireTimeout = \INF) { parent::__construct($name, $acquireTimeout); // TODO remove this BC once all tests fixed - if ($expireTimeout === (float) \PHP_INT_MAX) { + if ($expireTimeout === \INF) { $expireTimeout = $acquireTimeout; } diff --git a/src/Mutex/MemcachedMutex.php b/src/Mutex/MemcachedMutex.php index 9dda8136..4aa1bfba 100644 --- a/src/Mutex/MemcachedMutex.php +++ b/src/Mutex/MemcachedMutex.php @@ -20,7 +20,7 @@ class MemcachedMutex extends AbstractSpinlockExpireMutex * @param float $acquireTimeout In seconds * @param float $expireTimeout In seconds */ - public function __construct(string $name, \Memcached $memcached, float $acquireTimeout = 3, float $expireTimeout = \PHP_INT_MAX) + public function __construct(string $name, \Memcached $memcached, float $acquireTimeout = 3, float $expireTimeout = \INF) { parent::__construct($name, $acquireTimeout, $expireTimeout); diff --git a/tests/Mutex/AbstractRedlockMutexTest.php b/tests/Mutex/AbstractRedlockMutexTest.php index 301bc1f6..6573037e 100644 --- a/tests/Mutex/AbstractRedlockMutexTest.php +++ b/tests/Mutex/AbstractRedlockMutexTest.php @@ -45,7 +45,7 @@ protected function setUp(): void * * @return AbstractRedlockMutex&MockObject */ - private function createRedlockMutexMock(int $count, float $acquireTimeout = 1, float $expireTimeout = \PHP_INT_MAX): AbstractRedlockMutex + private function createRedlockMutexMock(int $count, float $acquireTimeout = 1, float $expireTimeout = \INF): AbstractRedlockMutex { $clients = array_map( static fn ($i) => new class($i) { diff --git a/tests/Mutex/AbstractSpinlockExpireMutexTest.php b/tests/Mutex/AbstractSpinlockExpireMutexTest.php index 1c0f3d80..1ffaab47 100644 --- a/tests/Mutex/AbstractSpinlockExpireMutexTest.php +++ b/tests/Mutex/AbstractSpinlockExpireMutexTest.php @@ -38,7 +38,7 @@ protected function setUp(): void /** * @return AbstractSpinlockExpireMutex&MockObject */ - private function createSpinlockExpireMutexMock(float $acquireTimeout = 3, float $expireTimeout = \PHP_INT_MAX): AbstractSpinlockExpireMutex + private function createSpinlockExpireMutexMock(float $acquireTimeout = 3, float $expireTimeout = \INF): AbstractSpinlockExpireMutex { return $this->getMockBuilder(AbstractSpinlockExpireMutex::class) ->setConstructorArgs(['test', $acquireTimeout, $expireTimeout]) From 351dbb07982f8147582fa9f17330250ff12147a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 16:30:14 +0100 Subject: [PATCH 08/24] no +1 for expire timeout --- src/Mutex/AbstractSpinlockExpireMutex.php | 11 +---------- tests/Mutex/RedisMutexWithPredisTest.php | 8 ++++---- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockExpireMutex.php index 88a2d5d4..5fea8043 100644 --- a/src/Mutex/AbstractSpinlockExpireMutex.php +++ b/src/Mutex/AbstractSpinlockExpireMutex.php @@ -42,16 +42,7 @@ protected function acquire(string $key): bool { $acquireTs = microtime(true); - /* - * The expiration timeout for the lock is increased by 1 second - * to ensure that we delete only our keys. This will prevent the - * case that this key expires before the timeout, and another process - * acquires successfully the same key which would then be deleted - * by this process. - * - * TODO 1 second should no longer be added as there are two separate timeouts newly - acquire and expire - */ - $token = $this->acquireWithToken($key, $this->expireTimeout + 1); + $token = $this->acquireWithToken($key, $this->expireTimeout); if ($token === false) { return false; diff --git a/tests/Mutex/RedisMutexWithPredisTest.php b/tests/Mutex/RedisMutexWithPredisTest.php index f6c3feb6..93f73a95 100644 --- a/tests/Mutex/RedisMutexWithPredisTest.php +++ b/tests/Mutex/RedisMutexWithPredisTest.php @@ -58,7 +58,7 @@ public function testAddFailsToSetKey(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') ->willReturn(null); $this->logger->expects(self::never()) @@ -80,7 +80,7 @@ public function testAddErrors(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') ->willThrowException($this->createMock(PredisException::class)); $this->logger->expects(self::once()) @@ -100,7 +100,7 @@ public function testWorksNormally(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') ->willReturnSelf(); $this->client->expects(self::once()) @@ -124,7 +124,7 @@ public function testEvalScriptFails(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 4500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') ->willReturnSelf(); $this->client->expects(self::once()) From bb51675ea1b856f452675e6a822fc26288cdb1b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 16:40:05 +0100 Subject: [PATCH 09/24] improve method mock cs --- tests/Mutex/AbstractSpinlockExpireMutexTest.php | 4 +++- tests/Mutex/AbstractSpinlockMutexTest.php | 16 ++++++++++++---- tests/Util/DoubleCheckedLockingTest.php | 3 ++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/tests/Mutex/AbstractSpinlockExpireMutexTest.php b/tests/Mutex/AbstractSpinlockExpireMutexTest.php index 1ffaab47..aba58df1 100644 --- a/tests/Mutex/AbstractSpinlockExpireMutexTest.php +++ b/tests/Mutex/AbstractSpinlockExpireMutexTest.php @@ -79,7 +79,9 @@ public function testExecuteTimeoutLeavesOneSecondForKeyToExpire(): void ->with(self::anything(), 1.3) ->willReturn('xx'); - $mutex->expects(self::once())->method('releaseWithToken')->willReturn(true); + $mutex->expects(self::once()) + ->method('releaseWithToken') + ->willReturn(true); $mutex->synchronized(static function () { usleep(199 * 1000); diff --git a/tests/Mutex/AbstractSpinlockMutexTest.php b/tests/Mutex/AbstractSpinlockMutexTest.php index b993edde..0e04e621 100644 --- a/tests/Mutex/AbstractSpinlockMutexTest.php +++ b/tests/Mutex/AbstractSpinlockMutexTest.php @@ -89,8 +89,12 @@ public function testAcquireTimeouts(): void public function testExecuteBarelySucceeds(): void { $mutex = $this->createSpinlockMutexMock(0.5); - $mutex->expects(self::any())->method('acquire')->willReturn(true); - $mutex->expects(self::once())->method('release')->willReturn(true); + $mutex->expects(self::any()) + ->method('acquire') + ->willReturn(true); + $mutex->expects(self::once()) + ->method('release') + ->willReturn(true); $mutex->synchronized(static function () { usleep(499 * 1000); @@ -105,8 +109,12 @@ public function testFailReleasingLock(): void $this->expectException(LockReleaseException::class); $mutex = $this->createSpinlockMutexMock(); - $mutex->expects(self::any())->method('acquire')->willReturn(true); - $mutex->expects(self::any())->method('release')->willReturn(false); + $mutex->expects(self::any()) + ->method('acquire') + ->willReturn(true); + $mutex->expects(self::any()) + ->method('release') + ->willReturn(false); $mutex->synchronized(static function () {}); } diff --git a/tests/Util/DoubleCheckedLockingTest.php b/tests/Util/DoubleCheckedLockingTest.php index b72786a3..2a848a97 100644 --- a/tests/Util/DoubleCheckedLockingTest.php +++ b/tests/Util/DoubleCheckedLockingTest.php @@ -25,7 +25,8 @@ protected function setUp(): void public function testCheckFailsAcquiresNoLock(): void { - $this->mutex->expects(self::never())->method('synchronized'); + $this->mutex->expects(self::never()) + ->method('synchronized'); $checkedLocking = new DoubleCheckedLocking($this->mutex, static function (): bool { return false; From f6332e8ad77d3872ded9555b066a2517ff6c7930 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 16:46:02 +0100 Subject: [PATCH 10/24] adjust AbstractSpinlockExpireMutexTest for new expire timeout --- .../Mutex/AbstractSpinlockExpireMutexTest.php | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/tests/Mutex/AbstractSpinlockExpireMutexTest.php b/tests/Mutex/AbstractSpinlockExpireMutexTest.php index aba58df1..2c2fea54 100644 --- a/tests/Mutex/AbstractSpinlockExpireMutexTest.php +++ b/tests/Mutex/AbstractSpinlockExpireMutexTest.php @@ -46,45 +46,41 @@ private function createSpinlockExpireMutexMock(float $acquireTimeout = 3, float ->getMock(); } - /** - * Tests executing code which exceeds the acquire timeout fails. - */ - public function testExecuteTooLong(): void + public function testExecuteExpireTimeout(): void { - $mutex = $this->createSpinlockExpireMutexMock(0.5); - $mutex->expects(self::any()) + $mutex = $this->createSpinlockExpireMutexMock(0.1, 0.2); + $mutex->expects(self::once()) ->method('acquireWithToken') + ->with(self::anything(), 0.2) ->willReturn('xx'); - $mutex->expects(self::any()) + $mutex->expects(self::once()) ->method('releaseWithToken') + ->with(self::anything(), 'xx') ->willReturn(true); - $this->expectException(ExecutionOutsideLockException::class); - $this->expectExceptionMessageMatches('~^The code executed for 0\.5\d+ seconds\. But the timeout is 0\.5 seconds. The last 0\.0\d+ seconds were executed outside of the lock\.$~'); - $mutex->synchronized(static function () { - usleep(501 * 1000); + usleep(199 * 1000); }); } - /** - * Tests executing exactly until the timeout will leave the key one more second. - */ - public function testExecuteTimeoutLeavesOneSecondForKeyToExpire(): void + public function testExecuteTooLong(): void { - $mutex = $this->createSpinlockExpireMutexMock(0.2, 0.3); - $mutex->expects(self::once()) + $mutex = $this->createSpinlockExpireMutexMock(0.1, 0.2); + $mutex->expects(self::any()) ->method('acquireWithToken') - ->with(self::anything(), 1.3) + ->with(self::anything(), 0.2) ->willReturn('xx'); - $mutex->expects(self::once()) + $mutex->expects(self::any()) ->method('releaseWithToken') ->willReturn(true); + $this->expectException(ExecutionOutsideLockException::class); + $this->expectExceptionMessageMatches('~^The code executed for 0\.2\d+ seconds\. But the timeout is 0\.2 seconds. The last 0\.0\d+ seconds were executed outside of the lock\.$~'); + $mutex->synchronized(static function () { - usleep(199 * 1000); + usleep(201 * 1000); }); } } From d76d251d54b30c5589189e429c052fc5d9fb327b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 18:17:30 +0100 Subject: [PATCH 11/24] adjust MemcachedMutexTest --- tests/Mutex/MemcachedMutexTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Mutex/MemcachedMutexTest.php b/tests/Mutex/MemcachedMutexTest.php index e7dbcdf6..58d5ce46 100644 --- a/tests/Mutex/MemcachedMutexTest.php +++ b/tests/Mutex/MemcachedMutexTest.php @@ -43,7 +43,7 @@ public function testFailAcquireLock(): void $this->memcached->expects(self::atLeastOnce()) ->method('add') - ->with('php-malkusch-lock:test', true, 2) + ->with('php-malkusch-lock:test', true, 1) ->willReturn(false); $this->mutex->synchronized(static function (): void { @@ -60,7 +60,7 @@ public function testFailReleasingLock(): void $this->memcached->expects(self::once()) ->method('add') - ->with('php-malkusch-lock:test', true, 2) + ->with('php-malkusch-lock:test', true, 1) ->willReturn(true); $this->memcached->expects(self::once()) From 9c6b10ac61b3b5618a6a6ccfe15d4b975d9a0a2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 18:29:22 +0100 Subject: [PATCH 12/24] adjust AbstractRedlockMutexTest::testAcquireTimeouts --- tests/Mutex/AbstractRedlockMutexTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Mutex/AbstractRedlockMutexTest.php b/tests/Mutex/AbstractRedlockMutexTest.php index 6573037e..c0cfb23d 100644 --- a/tests/Mutex/AbstractRedlockMutexTest.php +++ b/tests/Mutex/AbstractRedlockMutexTest.php @@ -202,8 +202,8 @@ public function testAcquireTimeouts(int $count, float $timeout, float $delay): v */ public static function provideAcquireTimeoutsCases(): iterable { - yield [1, 1.2 - 1, 1.201]; - yield [2, 1.2 - 1, 1.401]; + yield [1, 1.2, 1.201]; + yield [2, 20.4, 10.201]; } /** From 7fd8dc82814c98b8e0a2545b4073abde44c923ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 18:34:30 +0100 Subject: [PATCH 13/24] remove last todo --- src/Mutex/AbstractSpinlockExpireMutex.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockExpireMutex.php index 5fea8043..423bad9b 100644 --- a/src/Mutex/AbstractSpinlockExpireMutex.php +++ b/src/Mutex/AbstractSpinlockExpireMutex.php @@ -29,11 +29,6 @@ public function __construct(string $name, float $acquireTimeout = 3, float $expi { parent::__construct($name, $acquireTimeout); - // TODO remove this BC once all tests fixed - if ($expireTimeout === \INF) { - $expireTimeout = $acquireTimeout; - } - $this->expireTimeout = $expireTimeout; } From ad9568b90c4d483c39207848936447cf79fb40a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 19:03:23 +0100 Subject: [PATCH 14/24] adjust AbstractRedlockMutexTest::testAcquireTimeouts again --- tests/Mutex/AbstractRedlockMutexTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/Mutex/AbstractRedlockMutexTest.php b/tests/Mutex/AbstractRedlockMutexTest.php index c0cfb23d..807ee1de 100644 --- a/tests/Mutex/AbstractRedlockMutexTest.php +++ b/tests/Mutex/AbstractRedlockMutexTest.php @@ -182,7 +182,10 @@ public function testAcquireTimeouts(int $count, float $timeout, float $delay): v $this->expectException(LockAcquireTimeoutException::class); $this->expectExceptionMessage('Lock acquire timeout of ' . LockUtil::getInstance()->formatTimeout($timeout) . ' seconds has been exceeded'); - $mutex = $this->createRedlockMutexMock($count, $timeout); + $mutex = $this->createRedlockMutexMock($count, $timeout, $timeout); + $mutex->expects(self::exactly($count)) + ->method('evalScript') + ->willReturn(true); $mutex->expects(self::exactly($count)) ->method('add') From 5a113afe5d7a7d0c26ac4ba05a51158169bba521 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 19:13:43 +0100 Subject: [PATCH 15/24] adjust MemcachedMutexTest --- tests/Mutex/MemcachedMutexTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Mutex/MemcachedMutexTest.php b/tests/Mutex/MemcachedMutexTest.php index 58d5ce46..c85da6c0 100644 --- a/tests/Mutex/MemcachedMutexTest.php +++ b/tests/Mutex/MemcachedMutexTest.php @@ -31,7 +31,7 @@ protected function setUp(): void parent::setUp(); $this->memcached = $this->createMock(\Memcached::class); - $this->mutex = new MemcachedMutex('test', $this->memcached, 1); + $this->mutex = new MemcachedMutex('test', $this->memcached, 1, 2); } /** @@ -43,7 +43,7 @@ public function testFailAcquireLock(): void $this->memcached->expects(self::atLeastOnce()) ->method('add') - ->with('php-malkusch-lock:test', true, 1) + ->with('php-malkusch-lock:test', true, 2) ->willReturn(false); $this->mutex->synchronized(static function (): void { @@ -60,7 +60,7 @@ public function testFailReleasingLock(): void $this->memcached->expects(self::once()) ->method('add') - ->with('php-malkusch-lock:test', true, 1) + ->with('php-malkusch-lock:test', true, 2) ->willReturn(true); $this->memcached->expects(self::once()) From d1b03b3a5a167547f4e1712b1b67e7c8feb24297 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 8 Dec 2024 22:05:11 +0100 Subject: [PATCH 16/24] fix memcached TTL fully --- src/Mutex/MemcachedMutex.php | 27 ++++++++++++++++++++++----- tests/Mutex/MemcachedMutexTest.php | 4 ++-- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/Mutex/MemcachedMutex.php b/src/Mutex/MemcachedMutex.php index 4aa1bfba..c7f4e8ad 100644 --- a/src/Mutex/MemcachedMutex.php +++ b/src/Mutex/MemcachedMutex.php @@ -30,13 +30,9 @@ public function __construct(string $name, \Memcached $memcached, float $acquireT #[\Override] protected function acquireWithToken(string $key, float $expireTimeout) { - // memcached supports only integer expire - // https://github.com/memcached/memcached/wiki/Commands#standard-protocol - $expireTimeoutInt = LockUtil::getInstance()->castFloatToInt(ceil($expireTimeout)); - $token = LockUtil::getInstance()->makeRandomToken(); - return $this->memcached->add($key, $token, $expireTimeoutInt) + return $this->memcached->add($key, $token, $this->makeMemcachedExpireTimeout($expireTimeout)) ? $token : false; } @@ -48,4 +44,25 @@ protected function releaseWithToken(string $key, string $token): bool return $this->memcached->delete($key); } + + private function makeMemcachedExpireTimeout(float $value): int + { + $res = LockUtil::getInstance()->castFloatToInt(ceil($value)); + + // workaround https://github.com/memcached/memcached/issues/307 + if ($res < \PHP_INT_MAX) { + ++$res; + } + + // 0 means no expire + // https://github.com/php/doc-en/blob/af4410a7e1/reference/memcached/expiration.xml#L17 + $res = max(1, $res); + // >= 30 days means TS instead of TTL + // https://github.com/php/doc-en/blob/af4410a7e1/reference/memcached/expiration.xml#L12 + if ($res >= 30 * 24 * 60 * 60) { + $res = 0; + } + + return $res; + } } diff --git a/tests/Mutex/MemcachedMutexTest.php b/tests/Mutex/MemcachedMutexTest.php index c85da6c0..f2feb389 100644 --- a/tests/Mutex/MemcachedMutexTest.php +++ b/tests/Mutex/MemcachedMutexTest.php @@ -43,7 +43,7 @@ public function testFailAcquireLock(): void $this->memcached->expects(self::atLeastOnce()) ->method('add') - ->with('php-malkusch-lock:test', true, 2) + ->with('php-malkusch-lock:test', true, 3) ->willReturn(false); $this->mutex->synchronized(static function (): void { @@ -60,7 +60,7 @@ public function testFailReleasingLock(): void $this->memcached->expects(self::once()) ->method('add') - ->with('php-malkusch-lock:test', true, 2) + ->with('php-malkusch-lock:test', true, 3) ->willReturn(true); $this->memcached->expects(self::once()) From da292a2d5eb04b8d5b742b28f8591b0bdd615fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:12:30 +0100 Subject: [PATCH 17/24] fix Redis TTL fully --- src/Mutex/RedisMutex.php | 28 +++++++++++++++++++++--- tests/Mutex/RedisMutexWithPredisTest.php | 8 +++---- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/Mutex/RedisMutex.php b/src/Mutex/RedisMutex.php index d27aa988..90a9187d 100644 --- a/src/Mutex/RedisMutex.php +++ b/src/Mutex/RedisMutex.php @@ -35,18 +35,40 @@ private function isClientPHPRedis(object $client): bool return $res; } + private function makeRedisExpireTimeoutMillis(float $value): int + { + $res = LockUtil::getInstance()->castFloatToInt(ceil($value * 1000)); + + // workaround https://github.com/redis/docs/blob/377fb96c09/content/commands/expire/index.md?plain=1#L224 + if ($res < \PHP_INT_MAX) { + ++$res; + } + + // workaround time + timeout math overflow + if ($res < 0) { + $res = 0; + } elseif (\PHP_INT_SIZE >= 6) { + $thousandYearsMillis = (int) (1000 * 365.25 * 24 * 60 * 60 * 1000); + if ($res > $thousandYearsMillis) { + $res = $thousandYearsMillis; + } + } + + return $res; + } + /** * @throws LockAcquireException */ #[\Override] protected function add(object $client, string $key, string $value, float $expire): bool { - $expireMillis = LockUtil::getInstance()->castFloatToInt(ceil($expire * 1000)); + $expireTimeoutMillis = $this->makeRedisExpireTimeoutMillis($expire); if ($this->isClientPHPRedis($client)) { try { // Will set the key, if it doesn't exist, with a ttl of $expire seconds - return $client->set($key, $value, ['nx', 'px' => $expireMillis]); + return $client->set($key, $value, ['nx', 'px' => $expireTimeoutMillis]); } catch (\RedisException $e) { $message = sprintf( 'Failed to acquire lock for key \'%s\'', @@ -57,7 +79,7 @@ protected function add(object $client, string $key, string $value, float $expire } } else { try { - return $client->set($key, $value, 'PX', $expireMillis, 'NX') !== null; + return $client->set($key, $value, 'PX', $expireTimeoutMillis, 'NX') !== null; } catch (PredisException $e) { $message = sprintf( 'Failed to acquire lock for key \'%s\'', diff --git a/tests/Mutex/RedisMutexWithPredisTest.php b/tests/Mutex/RedisMutexWithPredisTest.php index 93f73a95..c35a3e11 100644 --- a/tests/Mutex/RedisMutexWithPredisTest.php +++ b/tests/Mutex/RedisMutexWithPredisTest.php @@ -58,7 +58,7 @@ public function testAddFailsToSetKey(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3501, 'NX') ->willReturn(null); $this->logger->expects(self::never()) @@ -80,7 +80,7 @@ public function testAddErrors(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3501, 'NX') ->willThrowException($this->createMock(PredisException::class)); $this->logger->expects(self::once()) @@ -100,7 +100,7 @@ public function testWorksNormally(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3501, 'NX') ->willReturnSelf(); $this->client->expects(self::once()) @@ -124,7 +124,7 @@ public function testEvalScriptFails(): void { $this->client->expects(self::atLeastOnce()) ->method('set') - ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3500, 'NX') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 3501, 'NX') ->willReturnSelf(); $this->client->expects(self::once()) From 20462fa8b5320e9497aeba3b00249f3c89f105d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:09:55 +0100 Subject: [PATCH 18/24] add test --- tests/Mutex/MemcachedMutexTest.php | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/tests/Mutex/MemcachedMutexTest.php b/tests/Mutex/MemcachedMutexTest.php index f2feb389..c79e1b2c 100644 --- a/tests/Mutex/MemcachedMutexTest.php +++ b/tests/Mutex/MemcachedMutexTest.php @@ -34,10 +34,7 @@ protected function setUp(): void $this->mutex = new MemcachedMutex('test', $this->memcached, 1, 2); } - /** - * Tests failing to acquire the lock within the timeout. - */ - public function testFailAcquireLock(): void + public function testAcquireFail(): void { $this->expectException(LockAcquireTimeoutException::class); @@ -51,10 +48,7 @@ public function testFailAcquireLock(): void }); } - /** - * Tests failing to release a lock. - */ - public function testFailReleasingLock(): void + public function testReleaseFail(): void { $this->expectException(LockReleaseException::class); @@ -70,4 +64,21 @@ public function testFailReleasingLock(): void $this->mutex->synchronized(static function (): void {}); } + + public function testAcquireExpireTimeoutLimit(): void + { + $this->mutex = new MemcachedMutex('test', $this->memcached); + + $this->memcached->expects(self::once()) + ->method('add') + ->with('php-malkusch-lock:test', true, 0) + ->willReturn(true); + + $this->memcached->expects(self::once()) + ->method('delete') + ->with('php-malkusch-lock:test') + ->willReturn(true); + + $this->mutex->synchronized(static function (): void {}); + } } From 60e39dd228b84fd9579f87bb17fe0b2b8ddd4bb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:20:50 +0100 Subject: [PATCH 19/24] fix test typo --- tests/Util/PcntlTimeoutTest.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/Util/PcntlTimeoutTest.php b/tests/Util/PcntlTimeoutTest.php index b0ce46c2..775c0378 100644 --- a/tests/Util/PcntlTimeoutTest.php +++ b/tests/Util/PcntlTimeoutTest.php @@ -7,11 +7,13 @@ use Malkusch\Lock\Exception\DeadlineException; use Malkusch\Lock\Exception\LockAcquireException; use Malkusch\Lock\Util\PcntlTimeout; +use PHPUnit\Framework\Attributes\RequiresPhpExtension; use PHPUnit\Framework\TestCase; /** - * @requires pcntl + * @requires extension pcntl */ +#[RequiresPhpExtension('pcntl')] class PcntlTimeoutTest extends TestCase { /** From bfb8f7a443d6275954d0244cf236181c660ae00e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:26:22 +0100 Subject: [PATCH 20/24] minor cs --- src/Util/PcntlTimeout.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Util/PcntlTimeout.php b/src/Util/PcntlTimeout.php index f4a22512..c066ee94 100644 --- a/src/Util/PcntlTimeout.php +++ b/src/Util/PcntlTimeout.php @@ -27,13 +27,11 @@ final class PcntlTimeout public function __construct(int $timeout) { if (!self::isSupported()) { - throw new \RuntimeException('PCNTL module not enabled'); + throw new \RuntimeException('PCNTL extension is required'); } if ($timeout <= 0) { - throw new \InvalidArgumentException( - 'Timeout must be positive and non zero' - ); + throw new \InvalidArgumentException('Timeout must be positive and non zero'); } $this->timeout = $timeout; @@ -94,8 +92,7 @@ public function timeBoxed(callable $code) */ public static function isSupported(): bool { - return - \PHP_SAPI === 'cli' + return \PHP_SAPI === 'cli' && extension_loaded('pcntl') && function_exists('pcntl_alarm') && function_exists('pcntl_signal') From 5f1c26112c6728f3fcdc9db7fde453983b1e877b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:40:55 +0100 Subject: [PATCH 21/24] add test --- tests/Mutex/RedisMutexWithPredisTest.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/Mutex/RedisMutexWithPredisTest.php b/tests/Mutex/RedisMutexWithPredisTest.php index c35a3e11..a69e509e 100644 --- a/tests/Mutex/RedisMutexWithPredisTest.php +++ b/tests/Mutex/RedisMutexWithPredisTest.php @@ -117,6 +117,23 @@ public function testWorksNormally(): void self::assertTrue($executed); } + public function testAcquireExpireTimeoutLimit(): void + { + $this->mutex = new RedisMutex([$this->client], 'test'); + + $this->client->expects(self::once()) + ->method('set') + ->with('php-malkusch-lock:test', new IsType(IsType::TYPE_STRING), 'PX', 31_557_600_000_000, 'NX') + ->willReturnSelf(); + + $this->client->expects(self::once()) + ->method('eval') + ->with(self::anything(), 1, 'php-malkusch-lock:test', new IsType(IsType::TYPE_STRING)) + ->willReturn(true); + + $this->mutex->synchronized(static function (): void {}); + } + /** * Tests evalScript() fails. */ From b456248e9865158d7a70887d2bc432bbfc70d826 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:43:58 +0100 Subject: [PATCH 22/24] rename new AbstractSpinlockExpireMutexTest class --- src/Mutex/AbstractRedlockMutex.php | 2 +- ...utex.php => AbstractSpinlockWithTokenMutex.php} | 2 +- src/Mutex/MemcachedMutex.php | 2 +- ....php => AbstractSpinlockWithTokenMutexTest.php} | 14 +++++++------- 4 files changed, 10 insertions(+), 10 deletions(-) rename src/Mutex/{AbstractSpinlockExpireMutex.php => AbstractSpinlockWithTokenMutex.php} (96%) rename tests/Mutex/{AbstractSpinlockExpireMutexTest.php => AbstractSpinlockWithTokenMutexTest.php} (81%) diff --git a/src/Mutex/AbstractRedlockMutex.php b/src/Mutex/AbstractRedlockMutex.php index 6683f323..e0524ea5 100644 --- a/src/Mutex/AbstractRedlockMutex.php +++ b/src/Mutex/AbstractRedlockMutex.php @@ -18,7 +18,7 @@ * * @see http://redis.io/topics/distlock */ -abstract class AbstractRedlockMutex extends AbstractSpinlockExpireMutex implements LoggerAwareInterface +abstract class AbstractRedlockMutex extends AbstractSpinlockWithTokenMutex implements LoggerAwareInterface { use LoggerAwareTrait; diff --git a/src/Mutex/AbstractSpinlockExpireMutex.php b/src/Mutex/AbstractSpinlockWithTokenMutex.php similarity index 96% rename from src/Mutex/AbstractSpinlockExpireMutex.php rename to src/Mutex/AbstractSpinlockWithTokenMutex.php index 423bad9b..bdc193c4 100644 --- a/src/Mutex/AbstractSpinlockExpireMutex.php +++ b/src/Mutex/AbstractSpinlockWithTokenMutex.php @@ -11,7 +11,7 @@ * * Lock is acquired with an unique token that is verified when the lock is being released. */ -abstract class AbstractSpinlockExpireMutex extends AbstractSpinlockMutex +abstract class AbstractSpinlockWithTokenMutex extends AbstractSpinlockMutex { /** In seconds */ private float $expireTimeout; diff --git a/src/Mutex/MemcachedMutex.php b/src/Mutex/MemcachedMutex.php index c7f4e8ad..d83b0f2f 100644 --- a/src/Mutex/MemcachedMutex.php +++ b/src/Mutex/MemcachedMutex.php @@ -9,7 +9,7 @@ /** * Memcached based spinlock implementation. */ -class MemcachedMutex extends AbstractSpinlockExpireMutex +class MemcachedMutex extends AbstractSpinlockWithTokenMutex { private \Memcached $memcached; diff --git a/tests/Mutex/AbstractSpinlockExpireMutexTest.php b/tests/Mutex/AbstractSpinlockWithTokenMutexTest.php similarity index 81% rename from tests/Mutex/AbstractSpinlockExpireMutexTest.php rename to tests/Mutex/AbstractSpinlockWithTokenMutexTest.php index 2c2fea54..aa741e4e 100644 --- a/tests/Mutex/AbstractSpinlockExpireMutexTest.php +++ b/tests/Mutex/AbstractSpinlockWithTokenMutexTest.php @@ -5,14 +5,14 @@ namespace Malkusch\Lock\Tests\Mutex; use Malkusch\Lock\Exception\ExecutionOutsideLockException; -use Malkusch\Lock\Mutex\AbstractSpinlockExpireMutex; +use Malkusch\Lock\Mutex\AbstractSpinlockWithTokenMutex; use phpmock\environment\SleepEnvironmentBuilder; use phpmock\MockEnabledException; use phpmock\phpunit\PHPMock; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -class AbstractSpinlockExpireMutexTest extends TestCase +class AbstractSpinlockWithTokenMutexTest extends TestCase { use PHPMock; @@ -36,11 +36,11 @@ protected function setUp(): void } /** - * @return AbstractSpinlockExpireMutex&MockObject + * @return AbstractSpinlockWithTokenMutex&MockObject */ - private function createSpinlockExpireMutexMock(float $acquireTimeout = 3, float $expireTimeout = \INF): AbstractSpinlockExpireMutex + private function createSpinlockWithTokenMutexMock(float $acquireTimeout = 3, float $expireTimeout = \INF): AbstractSpinlockWithTokenMutex { - return $this->getMockBuilder(AbstractSpinlockExpireMutex::class) + return $this->getMockBuilder(AbstractSpinlockWithTokenMutex::class) ->setConstructorArgs(['test', $acquireTimeout, $expireTimeout]) ->onlyMethods(['acquireWithToken', 'releaseWithToken']) ->getMock(); @@ -48,7 +48,7 @@ private function createSpinlockExpireMutexMock(float $acquireTimeout = 3, float public function testExecuteExpireTimeout(): void { - $mutex = $this->createSpinlockExpireMutexMock(0.1, 0.2); + $mutex = $this->createSpinlockWithTokenMutexMock(0.1, 0.2); $mutex->expects(self::once()) ->method('acquireWithToken') ->with(self::anything(), 0.2) @@ -66,7 +66,7 @@ public function testExecuteExpireTimeout(): void public function testExecuteTooLong(): void { - $mutex = $this->createSpinlockExpireMutexMock(0.1, 0.2); + $mutex = $this->createSpinlockWithTokenMutexMock(0.1, 0.2); $mutex->expects(self::any()) ->method('acquireWithToken') ->with(self::anything(), 0.2) From 4282ab14c44158166e558b098ae362a978f36d96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:46:57 +0100 Subject: [PATCH 23/24] improve param name --- src/Exception/ExecutionOutsideLockException.php | 12 ++++++------ tests/Mutex/AbstractSpinlockWithTokenMutexTest.php | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Exception/ExecutionOutsideLockException.php b/src/Exception/ExecutionOutsideLockException.php index c8bb70b7..54916eef 100644 --- a/src/Exception/ExecutionOutsideLockException.php +++ b/src/Exception/ExecutionOutsideLockException.php @@ -20,16 +20,16 @@ class ExecutionOutsideLockException extends LockReleaseException { /** - * @param float $elapsedTime Total elapsed time of the synchronized code callback execution - * @param float $timeout The lock timeout in seconds + * @param float $elapsedTime In seconds + * @param float $expireTimeout In seconds */ - public static function create(float $elapsedTime, float $timeout): self + public static function create(float $elapsedTime, float $expireTimeout): self { return new self(\sprintf( - 'The code executed for %s seconds. But the timeout is %s seconds. The last %s seconds were executed outside of the lock.', + 'The code executed for %s seconds. But the expire timeout is %s seconds. The last %s seconds were executed outside of the lock.', LockUtil::getInstance()->formatTimeout($elapsedTime), - LockUtil::getInstance()->formatTimeout($timeout), - LockUtil::getInstance()->formatTimeout(round($elapsedTime, 6) - round($timeout, 6)) + LockUtil::getInstance()->formatTimeout($expireTimeout), + LockUtil::getInstance()->formatTimeout(round($elapsedTime, 6) - round($expireTimeout, 6)) )); } } diff --git a/tests/Mutex/AbstractSpinlockWithTokenMutexTest.php b/tests/Mutex/AbstractSpinlockWithTokenMutexTest.php index aa741e4e..38810c58 100644 --- a/tests/Mutex/AbstractSpinlockWithTokenMutexTest.php +++ b/tests/Mutex/AbstractSpinlockWithTokenMutexTest.php @@ -77,7 +77,7 @@ public function testExecuteTooLong(): void ->willReturn(true); $this->expectException(ExecutionOutsideLockException::class); - $this->expectExceptionMessageMatches('~^The code executed for 0\.2\d+ seconds\. But the timeout is 0\.2 seconds. The last 0\.0\d+ seconds were executed outside of the lock\.$~'); + $this->expectExceptionMessageMatches('~^The code executed for 0\.2\d+ seconds\. But the expire timeout is 0\.2 seconds. The last 0\.0\d+ seconds were executed outside of the lock\.$~'); $mutex->synchronized(static function () { usleep(201 * 1000); From 76e175b941ca981f24ff7c41ab2b46feee67d7e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 9 Dec 2024 00:55:24 +0100 Subject: [PATCH 24/24] minor theoretical improvement --- src/Mutex/MemcachedMutex.php | 2 +- src/Mutex/RedisMutex.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mutex/MemcachedMutex.php b/src/Mutex/MemcachedMutex.php index d83b0f2f..aeccff0a 100644 --- a/src/Mutex/MemcachedMutex.php +++ b/src/Mutex/MemcachedMutex.php @@ -50,7 +50,7 @@ private function makeMemcachedExpireTimeout(float $value): int $res = LockUtil::getInstance()->castFloatToInt(ceil($value)); // workaround https://github.com/memcached/memcached/issues/307 - if ($res < \PHP_INT_MAX) { + if ($res > 0 && $res < \PHP_INT_MAX) { ++$res; } diff --git a/src/Mutex/RedisMutex.php b/src/Mutex/RedisMutex.php index 90a9187d..19caf68d 100644 --- a/src/Mutex/RedisMutex.php +++ b/src/Mutex/RedisMutex.php @@ -40,7 +40,7 @@ private function makeRedisExpireTimeoutMillis(float $value): int $res = LockUtil::getInstance()->castFloatToInt(ceil($value * 1000)); // workaround https://github.com/redis/docs/blob/377fb96c09/content/commands/expire/index.md?plain=1#L224 - if ($res < \PHP_INT_MAX) { + if ($res > 0 && $res < \PHP_INT_MAX) { ++$res; }