Add fail callback support in DoubleCheckedLocking::then() method#76
Add fail callback support in DoubleCheckedLocking::then() method#76
Conversation
| { | ||
| parent::__construct('', $acquireTimeout, $expireTimeout); | ||
| \Closure::bind(function () { | ||
| $this->key = 'distributed'; |
There was a problem hiding this comment.
@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 $mutexesto ensure their keys are all exactly the same. - Then, use that key as the
DistributedMutexkey instead of a fixed stringdistributed.
There was a problem hiding this comment.
Does the key really matter, where exactly it is used?
- Loop through
array $mutexesto 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
close #11