From 413e1cd3a33d6ecf11e1a9b97d1aa68d117d28b6 Mon Sep 17 00:00:00 2001 From: Julian Vennen Date: Fri, 4 Apr 2025 17:24:26 +0200 Subject: [PATCH 1/3] Split etcd storage logic from Lock class --- src/AbstractLock.php | 3 ++ src/Lock.php | 91 +++++++++++++++----------------- src/Storage/EtcdStorage.php | 47 +++++++++++++++++ src/Storage/StorageException.php | 13 +++++ src/Storage/StorageInterface.php | 44 +++++++++++++++ test/LockTest.php | 91 ++++++-------------------------- 6 files changed, 164 insertions(+), 125 deletions(-) create mode 100644 src/Storage/EtcdStorage.php create mode 100644 src/Storage/StorageException.php create mode 100644 src/Storage/StorageInterface.php diff --git a/src/AbstractLock.php b/src/AbstractLock.php index 4cb4c02..1936a6e 100644 --- a/src/AbstractLock.php +++ b/src/AbstractLock.php @@ -2,6 +2,9 @@ namespace Aternos\Lock; +/** + * Abstract base class for locks that contains getters and setters for the lock properties. + */ abstract class AbstractLock implements LockInterface { /** diff --git a/src/Lock.php b/src/Lock.php index e056ad4..51d55f6 100644 --- a/src/Lock.php +++ b/src/Lock.php @@ -2,26 +2,23 @@ namespace Aternos\Lock; -use Aternos\Etcd\Client; -use Aternos\Etcd\ClientInterface; -use Aternos\Etcd\Exception\Status\DeadlineExceededException; -use Aternos\Etcd\Exception\Status\InvalidResponseStatusCodeException; -use Aternos\Etcd\Exception\Status\UnavailableException; -use Aternos\Etcd\Exception\Status\UnknownException; +use Aternos\Lock\Storage\EtcdStorage; +use Aternos\Lock\Storage\StorageException; +use Aternos\Lock\Storage\StorageInterface; +use Exception; /** - * Class Lock + * LockInterface implementation using etcd-like storage * * @package Aternos\Lock */ class Lock extends AbstractLock { /** - * see Lock::setClient() - * - * @var ClientInterface|null + * @see static::setStorage() + * @var StorageInterface|null */ - protected static ?ClientInterface $client = null; + protected static ?StorageInterface $storage = null; /** * see Lock::setPrefix() @@ -66,15 +63,22 @@ class Lock extends AbstractLock protected static int $delayPerUnavailableRetry = 1; /** - * Set the etcd client (Aternos\Etcd\Client) - * - * Uses a localhost client if not set - * - * @param ClientInterface $client + * Set the storage interface used to store locks. If not set, {@link EtcdStorage} is used. + * @param StorageInterface $storage + * @return void + */ + public static function setStorage(StorageInterface $storage): void + { + static::$storage = $storage; + } + + /** + * Get the storage interface used to store locks. If not set, {@link EtcdStorage} is used. + * @return StorageInterface */ - public static function setClient(ClientInterface $client): void + protected static function getStorage(): StorageInterface { - static::$client = $client; + return static::$storage ??= new EtcdStorage(); } /** @@ -110,12 +114,13 @@ public static function setMaxSaveRetries(int $retries): void } /** - * Set the maximum delay in microseconds (1,000,000 microseconds = 1 second) that should used for the random delay between retries + * Set the maximum delay in microseconds (1,000,000 microseconds = 1 second) that should be used for the random + * delay between retries. * * The delay is random and calculated like this: rand(0, $retries * $delayPerRetry) * * Lower value = faster retries (probably more retries necessary) - * Higher value = slower retries (probably less retries necessary) + * Higher value = slower retries (probably fewer retries necessary) * * @param int $delayPerRetry */ @@ -144,20 +149,6 @@ public static function setDelayPerUnavailableRetry(int $delayPerRetry): void static::$delayPerUnavailableRetry = $delayPerRetry; } - /** - * Get an Aternos\Etcd\Client instance - * - * @return ClientInterface - */ - protected static function getClient(): ClientInterface - { - if (static::$client === null) { - static::$client = new Client(); - } - - return static::$client; - } - /** * Full name of the key in etcd (prefix + key) * @@ -222,7 +213,7 @@ public function __construct( * Try to acquire lock * * @return bool true if the lock was acquired, false if it was not - * @throws InvalidResponseStatusCodeException + * @throws StorageException * @throws TooManySaveRetriesException */ public function lock(): bool @@ -247,7 +238,7 @@ public function lock(): bool * * @param int|null $waitTime maximum time in seconds to wait for other locks * @return bool - * @throws InvalidResponseStatusCodeException + * @throws StorageException */ public function waitForOtherLocks(?int $waitTime = null): bool { @@ -284,7 +275,7 @@ public function getRemainingLockDuration(): int * Refresh the lock * * @return boolean - * @throws InvalidResponseStatusCodeException + * @throws StorageException * @throws TooManySaveRetriesException */ public function refresh(): bool @@ -312,7 +303,7 @@ public function refresh(): bool * Should be only used if you have the lock * * @return void - * @throws InvalidResponseStatusCodeException + * @throws StorageException * @throws TooManySaveRetriesException */ public function break(): void @@ -340,7 +331,7 @@ protected function generateLock(): LockEntry * Remove a lock from the locking array and save the locks * * @return void - * @throws InvalidResponseStatusCodeException + * @throws StorageException * @throws TooManySaveRetriesException */ protected function removeLock(): void @@ -362,7 +353,7 @@ protected function removeLock(): void * * @param int $time * @return bool - * @throws InvalidResponseStatusCodeException + * @throws StorageException * @throws TooManySaveRetriesException */ protected function addOrUpdateLock(int $time): bool @@ -389,8 +380,9 @@ protected function addOrUpdateLock(int $time): bool * changed since the last update, they will be updated by this function again. * * @return bool - * @throws InvalidResponseStatusCodeException + * @throws StorageException * @throws TooManySaveRetriesException + * @throws Exception */ protected function saveLocks(): bool { @@ -408,9 +400,9 @@ protected function saveLocks(): bool if (count($this->locks) === 0) { for ($i = 1; $i <= static::$maxUnavailableRetries; $i++) { try { - $result = static::getClient()->deleteIf($this->etcdKey, $previousLocks, !$delayRetry); + $result = static::getStorage()->deleteIf($this->etcdKey, $previousLocks, !$delayRetry); break; - } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { + } catch (StorageException $e) { if ($i === static::$maxUnavailableRetries) { throw $e; } else { @@ -424,9 +416,9 @@ protected function saveLocks(): bool for ($i = 1; $i <= static::$maxUnavailableRetries; $i++) { try { - $result = static::getClient()->putIf($this->etcdKey, $lockString, $previousLocks, !$delayRetry); + $result = static::getStorage()->putIf($this->etcdKey, $lockString, $previousLocks, !$delayRetry); break; - } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { + } catch (StorageException $e) { if ($i === static::$maxUnavailableRetries) { throw $e; } else { @@ -479,17 +471,18 @@ protected function canLock(): bool /** * Update the locks array from etcd * - * @throws InvalidResponseStatusCodeException * @return $this + * @throws StorageException + * @throws Exception */ public function update(): static { $etcdLockString = false; for ($i = 1; $i <= static::$maxUnavailableRetries; $i++) { try { - $etcdLockString = static::getClient()->get($this->etcdKey); + $etcdLockString = static::getStorage()->get($this->etcdKey); break; - } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { + } catch (StorageException $e) { if ($i === static::$maxUnavailableRetries) { throw $e; } else { @@ -524,7 +517,7 @@ protected function updateFromString(string|bool $lockString): static /** * Break the lock on destruction of this object * - * @throws InvalidResponseStatusCodeException + * @throws StorageException * @throws TooManySaveRetriesException */ public function __destruct() diff --git a/src/Storage/EtcdStorage.php b/src/Storage/EtcdStorage.php new file mode 100644 index 0000000..cd06710 --- /dev/null +++ b/src/Storage/EtcdStorage.php @@ -0,0 +1,47 @@ +client = $client ?? new Client(); + } + + public function putIf(string $key, string $value, bool|string $previousValue, bool $returnNewValueOnFail): bool|string + { + try { + return $this->client->putIf($key, $value, $previousValue, $returnNewValueOnFail); + } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { + throw new StorageException($e->getMessage(), $e->getCode(), $e); + } + } + + public function deleteIf(string $key, $previousValue, bool $returnNewValueOnFail = false): bool|string + { + try { + return $this->client->deleteIf($key, $previousValue, $returnNewValueOnFail); + } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { + throw new StorageException($e->getMessage(), $e->getCode(), $e); + } + } + + + public function get(string $key): bool|string + { + try { + return $this->client->get($key); + } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { + throw new StorageException($e->getMessage(), $e->getCode(), $e); + } + } +} diff --git a/src/Storage/StorageException.php b/src/Storage/StorageException.php new file mode 100644 index 0000000..a70df12 --- /dev/null +++ b/src/Storage/StorageException.php @@ -0,0 +1,13 @@ +assertEquals("identifier", $lock->getIdentifier()); } - public function testGetkey(): void + public function testGetKey(): void { $lock = new Lock("key"); $this->assertEquals("key", $lock->getKey()); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ public function testBreakLock(): void { $key = $this->getRandomString(); @@ -93,10 +90,6 @@ public function testBreakLockTwice(): void $breakingLock->break(); } - /** - * @throws InvalidResponseStatusCodeException - * @throws TooManySaveRetriesException - */ public function testAutoReleaseLock(): void { $key = $this->getRandomString(); @@ -111,10 +104,7 @@ public function testAutoReleaseLock(): void $lock->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testRefreshLock(): void { $key = $this->getRandomString(); @@ -132,10 +122,7 @@ public function testRefreshLock(): void $this->assertFalse($lock->isLocked()); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testRefreshLockThreshold(): void { $key = $this->getRandomString(); @@ -155,10 +142,7 @@ public function testRefreshLockThreshold(): void $this->assertFalse($lock->isLocked()); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testMultipleSharedLocks(): void { $key = $this->getRandomString(); @@ -194,10 +178,7 @@ public function testMultipleSharedLocks(): void $lockC->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testExclusiveLock(): void { $key = $this->getRandomString(); @@ -212,10 +193,7 @@ public function testExclusiveLock(): void $lock->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testWaitForExclusiveLockAfterExclusiveLock(): void { $key = $this->getRandomString(); @@ -234,10 +212,7 @@ public function testWaitForExclusiveLockAfterExclusiveLock(): void $lockB->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testRejectSharedLockWhileExclusiveLock(): void { $key = $this->getRandomString(); @@ -255,10 +230,7 @@ public function testRejectSharedLockWhileExclusiveLock(): void $lockB->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testRejectExclusiveLockWhileSharedLock(): void { $key = $this->getRandomString(); @@ -276,10 +248,7 @@ public function testRejectExclusiveLockWhileSharedLock(): void $lockB->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testWaitForExclusiveLockAfterMultipleSharedLocks(): void { $key = $this->getRandomString(); @@ -311,10 +280,7 @@ public function testWaitForExclusiveLockAfterMultipleSharedLocks(): void $lockD->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testLockWriteConflict(): void { $key = $this->getRandomString(); @@ -346,10 +312,7 @@ public function testLockWriteConflict(): void $lockB->break(); } - /** - * @throws TooManySaveRetriesException - * @throws InvalidResponseStatusCodeException - */ + public function testLockDeleteConflict(): void { $key = $this->getRandomString(); @@ -378,10 +341,6 @@ public function testLockDeleteConflict(): void $lockB->break(); } - /** - * @throws InvalidResponseStatusCodeException - * @throws TooManySaveRetriesException - */ public function testLockFunctionUsesUniqueDefaultIdentifierIfNoIdentifierParameterIsProvided(): void { # Default identifier is not set explicitly and its default value is null. @@ -402,10 +361,6 @@ public function testLockFunctionUsesUniqueDefaultIdentifierIfNoIdentifierParamet $lock->break(); } - /** - * @throws InvalidResponseStatusCodeException - * @throws TooManySaveRetriesException - */ public function testLockFunctionUsesThePresetDefaultIdentifierIfNoIdentifierParameterIsProvided(): void { # Default identifier is set @@ -416,10 +371,6 @@ public function testLockFunctionUsesThePresetDefaultIdentifierIfNoIdentifierPara $lock->break(); } - /** - * @throws InvalidResponseStatusCodeException - * @throws TooManySaveRetriesException - */ public function testBreaksOnDestructWhenBreakOnDestructPropertyIsTrue(): void { # Check that break() and isLocked() are called when breakOnDestruct is set to true @@ -435,10 +386,6 @@ public function testBreaksOnDestructWhenBreakOnDestructPropertyIsTrue(): void $breakingLock->__destruct(); } - /** - * @throws InvalidResponseStatusCodeException - * @throws TooManySaveRetriesException - */ public function testDoesNotBreakOnDestructWhenBreakOnDestructPropertyIsFalse(): void { # Check that break() and isLocked() are not called when breakOnDestruct is set to false @@ -458,10 +405,6 @@ public function testDoesNotBreakOnDestructWhenBreakOnDestructPropertyIsFalse(): $notBreakingLock->__destruct(); } - /** - * @throws InvalidResponseStatusCodeException - * @throws TooManySaveRetriesException - */ public function testCanLockWithSameKeyTwiceWhenIsNotExclusive(): void { $key = $this->getRandomString(); @@ -493,10 +436,6 @@ public function testCanLockWithSameKeyTwiceWhenIsNotExclusive(): void $this->assertFalse($lockB->isLockedByOther()); } - /** - * @throws InvalidResponseStatusCodeException - * @throws TooManySaveRetriesException - */ public function testCannotLockWithSameKeyTwiceWhenIsExclusive(): void { $key = $this->getRandomString(); From 4fb55ef84689a4dd2cec4ea1f5fa5024d93b4e5c Mon Sep 17 00:00:00 2001 From: Julian Vennen Date: Fri, 4 Apr 2025 19:40:27 +0200 Subject: [PATCH 2/3] Use ?string instead of false|string --- src/Storage/EtcdStorage.php | 18 ++++++++++++------ src/Storage/StorageInterface.php | 25 +++++++++++++++---------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/Storage/EtcdStorage.php b/src/Storage/EtcdStorage.php index cd06710..6ef54bd 100644 --- a/src/Storage/EtcdStorage.php +++ b/src/Storage/EtcdStorage.php @@ -17,31 +17,37 @@ public function __construct(?ClientInterface $client = null) $this->client = $client ?? new Client(); } - public function putIf(string $key, string $value, bool|string $previousValue, bool $returnNewValueOnFail): bool|string + public function putIf(string $key, string $value, ?string $previousValue, bool $returnNewValueOnFail): bool|string { try { - return $this->client->putIf($key, $value, $previousValue, $returnNewValueOnFail); + return $this->client->putIf($key, $value, $previousValue ?? false, $returnNewValueOnFail); } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { throw new StorageException($e->getMessage(), $e->getCode(), $e); } } - public function deleteIf(string $key, $previousValue, bool $returnNewValueOnFail = false): bool|string + public function deleteIf(string $key, ?string $previousValue, bool $returnNewValueOnFail = false): bool|string { try { - return $this->client->deleteIf($key, $previousValue, $returnNewValueOnFail); + return $this->client->deleteIf($key, $previousValue ?? false, $returnNewValueOnFail); } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { throw new StorageException($e->getMessage(), $e->getCode(), $e); } } - public function get(string $key): bool|string + public function get(string $key): ?string { try { - return $this->client->get($key); + $value = $this->client->get($key); } catch (UnavailableException | DeadlineExceededException | UnknownException $e) { throw new StorageException($e->getMessage(), $e->getCode(), $e); } + + if ($value === false) { + return null; + } + + return $value; } } diff --git a/src/Storage/StorageInterface.php b/src/Storage/StorageInterface.php index 34f7373..09515bd 100644 --- a/src/Storage/StorageInterface.php +++ b/src/Storage/StorageInterface.php @@ -10,35 +10,40 @@ interface StorageInterface { /** - * Put `$value` if `$key` value matches `$previousValue` otherwise `$returnNewValueOnFail` + * Put `$value` if `$key` value matches `$previousValue` and return true. If the new value does not match and + * `$returnNewValueOnFail` is true return the new value, otherwise return false. * @param string $key * @param string $value The new value to set - * @param bool|string $previousValue The previous value to compare against - * @param bool $returnNewValueOnFail - * @return bool|string + * @param string|null $previousValue The previous value to compare against. If null is provided, the comparison + * should check that the key does not exist yet. + * @param bool $returnNewValueOnFail if true the new value of the key should be returned if the operation fails + * @return bool|string true if the operation succeeded, false if it failed and `$returnNewValueOnFail` is false, + * otherwise the new value of the key * @throws StorageException a known, retryable error occurred * @throws Exception an unknown error occurred */ - public function putIf(string $key, string $value, bool|string $previousValue, bool $returnNewValueOnFail): bool|string; + public function putIf(string $key, string $value, ?string $previousValue, bool $returnNewValueOnFail): bool|string; /** * Delete if $key value matches $previous value otherwise $returnNewValueOnFail * * @param string $key - * @param bool|string $previousValue The previous value to compare against + * @param string|null $previousValue The previous value to compare against. If null is provided, the comparison + * should check that the key does not exist yet. * @param bool $returnNewValueOnFail - * @return bool|string + * @return bool|string true if the operation succeeded, false if it failed and `$returnNewValueOnFail` is false, + * otherwise the new value of the key * @throws StorageException a known, retryable error occurred * @throws Exception an unknown error occurred */ - public function deleteIf(string $key, bool|string $previousValue, bool $returnNewValueOnFail = false): bool|string; + public function deleteIf(string $key, ?string $previousValue, bool $returnNewValueOnFail = false): bool|string; /** * Get the value of a key * @param string $key - * @return bool|string + * @return string|null the value of the key or null if the key does not exist * @throws StorageException a known, retryable error occurred * @throws Exception an unknown error occurred */ - public function get(string $key): bool|string; + public function get(string $key): ?string; } From d2e6e440c3e511ccc6a64083154ab5476fb504af Mon Sep 17 00:00:00 2001 From: Julian Vennen Date: Fri, 4 Apr 2025 19:42:25 +0200 Subject: [PATCH 3/3] Update usages of storage interface to use nullable strings --- src/Lock.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Lock.php b/src/Lock.php index 51d55f6..752fcc5 100644 --- a/src/Lock.php +++ b/src/Lock.php @@ -162,9 +162,9 @@ public static function setDelayPerUnavailableRetry(int $delayPerRetry): void * Will be used in deleteIf and putIf requests to check * if there was no change in etcd while processing the lock * - * @var string|bool + * @var string */ - protected string|bool $previousLockString = false; + protected ?string $previousLockString = null; /** * Current parsed locks @@ -498,10 +498,10 @@ public function update(): static /** * Update the locks array from a JSON string * - * @param string|bool $lockString + * @param ?string $lockString * @return $this */ - protected function updateFromString(string|bool $lockString): static + protected function updateFromString(?string $lockString): static { $this->previousLockString = $lockString;