Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 6 additions & 16 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,9 @@ $newBalance = $mutex->check(static function () use ($bankAccount, $amount): bool

return $balance;
});

if (!$newBalance) {
if ($balance < 0) {
throw new \DomainException('You have no credit');
}
}
```

### Extracting code result after lock release exception
### LockReleaseException::getCode{Exception, Result}()

Mutex implementations based on [`Malkush\Lock\Mutex\AbstractLockMutex`][10] will throw
[`Malkusch\Lock\Exception\LockReleaseException`][11] in case of lock release
Expand All @@ -134,25 +128,21 @@ In order to read the code result (or an exception thrown there),
Example:
```php
try {
// OR $mutex->check(...)
$result = $mutex->synchronized(static function () {
if (someCondition()) {
throw new \DomainException();
}

return 'result';
});
} catch (LockReleaseException $unlockException) {
if ($unlockException->getCodeException() !== null) {
$codeException = $unlockException->getCodeException();
// do something with the code exception
} catch (LockReleaseException $e) {
if ($e->getCodeException() !== null) {
// do something with the $e->getCodeException() exception
} else {
$codeResult = $unlockException->getCodeResult();
// do something with the code result
// do something with the $e->getCodeResult() result
}

// deal with LockReleaseException or propagate it
throw $unlockException;
throw $e;
}
```

Expand Down
9 changes: 2 additions & 7 deletions src/Exception/LockReleaseException.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
*
* Take this exception very serious.
*
* This exception implies that the critical code was executed, i.e. side effects may have happened.
* This exception implies that the synchronized code was executed, i.e. side effects may have happened.
*
* Failing to release a lock might have the potential to introduce deadlocks.
* Failing to release a lock might also introduce deadlock.
*/
class LockReleaseException extends MutexException
{
Expand All @@ -21,8 +21,6 @@ class LockReleaseException extends MutexException
private ?\Throwable $codeException = null;

/**
* The return value of the executed critical code.
*
* @return mixed
*/
public function getCodeResult()
Expand All @@ -42,9 +40,6 @@ public function setCodeResult($codeResult): self
return $this;
}

/**
* The exception that has happened during the critical code execution.
*/
public function getCodeException(): ?\Throwable
{
return $this->codeException;
Expand Down
3 changes: 3 additions & 0 deletions src/Mutex/DistributedMutex.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ class DistributedMutex extends AbstractSpinlockWithTokenMutex implements LoggerA
public function __construct(array $mutexes, float $acquireTimeout = 3, float $expireTimeout = \INF)
{
parent::__construct('', $acquireTimeout, $expireTimeout);
\Closure::bind(function () {
$this->key = 'distributed';
Copy link

@mollierobbert mollierobbert Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvorisek Why did you decide to pin the key to this value?

This key is now used for the lock across all underlying mutex clients, overriding the key provided to each client in their constructor. Is this intentional?

Would it not make more sense to do the following?

  • Loop through array $mutexes to ensure their keys are all exactly the same.
  • Then, use that key as the DistributedMutex key instead of a fixed string distributed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the key really matter, where exactly it is used?

  • Loop through array $mutexes to ensure their keys are all exactly the same.

Does requiring the keys to be the same provide any safety advantage?

In anycase, improvement pull requests are welcomed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the key is passed to the mutexes, which then use it to acquire the lock (and later on, release it).

So let's say you do this:

$mutex = new DistributedMutex([
    new RedisMutex($redisInstance1,  "my-key", ...),
    new RedisMutex($redisInstance2,  "my-key", ...),
]);

$mutex->synchronized($fn);

At Redis level, the lock is not named my-key, but rather it is always called distributed.

If in another process I try to lock my-other-key, it will not work. Because it will have to wait for the same distributed lock to be released.

I may be misinterpreting either the implementation or the intended use of this unchangeable key though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If distributed is used for anything than internal purposes, it really does not seems right/intended.

Please verify and if this is true, please send a PR with a test.

}, $this, AbstractSpinlockMutex::class)();

$this->mutexes = $mutexes;
$this->logger = new NullLogger();
Expand Down
43 changes: 26 additions & 17 deletions src/Util/DoubleCheckedLocking.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,44 +21,53 @@ class DoubleCheckedLocking
private Mutex $mutex;

/** @var callable(): bool */
private $check;
private $checkFx;

/**
* @param callable(): bool $check Callback that decides if the lock should be acquired and is rechecked
* after a lock has been acquired
* @param callable(): bool $checkFx Decides if a lock should be acquired and is rechecked after the lock has been acquired
*/
public function __construct(Mutex $mutex, callable $check)
public function __construct(Mutex $mutex, callable $checkFx)
{
$this->mutex = $mutex;
$this->check = $check;
$this->checkFx = $checkFx;
}

private function invokeCheckFx(): bool
{
return ($this->checkFx)();
}

/**
* Executes a block of code only after the check callback passes
* before and after acquiring a lock.
* Execute a block of code only after the check callback passes before and after acquiring a lock.
*
* @template T
* @template TSuccess
* @template TFail = never
*
* @param callable(): T $code
* @param callable(): TSuccess $successFx
* @param callable(): TFail $failFx
*
* @return T|false False if check did not pass
* @return TSuccess|($failFx is null ? false : TFail)
*
* @throws \Throwable
* @throws LockAcquireException
* @throws LockReleaseException
*/
public function then(callable $code)
public function then(callable $successFx, ?callable $failFx = null)
{
if (!($this->check)()) {
return false;
if (!$this->invokeCheckFx()) {
return $failFx !== null
? $failFx()
: false;
}

return $this->mutex->synchronized(function () use ($code) {
if (!($this->check)()) {
return false;
return $this->mutex->synchronized(function () use ($successFx, $failFx) {
if (!$this->invokeCheckFx()) {
return $failFx !== null
? $failFx()
: false;
}

return $code();
return $successFx();
});
}
}
4 changes: 2 additions & 2 deletions tests/Mutex/DistributedMutexTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ public function testAcquireMutexLogger(): void

$mutex->expects(self::exactly(3))
->method('acquireMutex')
->with(self::isInstanceOf(AbstractSpinlockMutex::class), 'php-malkusch-lock:', 1.0, \INF)
->with(self::isInstanceOf(AbstractSpinlockMutex::class), 'distributed', 1.0, \INF)
->willThrowException($this->createMock(/* PredisException::class */ LockAcquireException::class));

$logger->expects(self::exactly(3))
Expand All @@ -357,7 +357,7 @@ public function testReleaseMutexLogger(): void

$mutex->expects(self::exactly(3))
->method('releaseMutex')
->with(self::isInstanceOf(AbstractSpinlockMutex::class), 'php-malkusch-lock:', \INF)
->with(self::isInstanceOf(AbstractSpinlockMutex::class), 'distributed', \INF)
->willThrowException($this->createMock(/* PredisException::class */ LockReleaseException::class));

$logger->expects(self::exactly(3))
Expand Down
46 changes: 46 additions & 0 deletions tests/Util/DoubleCheckedLockingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,50 @@ public function testCodeExecuted(): void
self::assertSame(1, $executedCount);
self::assertSame('foo', $result);
}

public function testFailCodeExecutedBeforeLock(): void
{
$this->mutex->expects(self::never())
->method('synchronized');

$checkedLocking = new DoubleCheckedLocking($this->mutex, static function () {
return false;
});

$executedCount = 0;
$result = $checkedLocking->then(static function () {
self::fail();
}, static function () use (&$executedCount) {
++$executedCount;

return 'foo';
});

self::assertSame(1, $executedCount);
self::assertSame('foo', $result);
}

public function testFailCodeExecutedAfterLock(): void
{
$this->mutex->expects(self::once())
->method('synchronized')
->willReturnCallback(static fn (\Closure $block) => $block());

$i = 0;
$checkedLocking = new DoubleCheckedLocking($this->mutex, static function () use (&$i) {
return $i++ === 0;
});

$executedCount = 0;
$result = $checkedLocking->then(static function () {
self::fail();
}, static function () use (&$executedCount) {
++$executedCount;

return 'foo';
});

self::assertSame(1, $executedCount);
self::assertSame('foo', $result);
}
}