Add Predis timeout #31
Conversation
|
Hi @arvenil Thanks for this package. I see you've made some significant changes in master. If there is any chance of this PR getting merged I will update it to reflect those changes. Could you let me know if it's worth me spending the time on that? Currently I have merged master into my branch but due to the changes it's broken and should not be merged. Thanks, |
|
Few months ago I merged this PR manually ( you can see it from https://github.com/deminy/ninja-mutex ) and used it in some production environment for data migration (for tens of millions of records at least). Everything worked fine so I'd recommend to accept this PR. Thanks |
src/Lock/LockAbstract.php
Outdated
| try { | ||
| $this->releaseLock($name); | ||
| } catch (Exception $e) { | ||
| // Ignoring exceptions in destructor to not block shutdown. | ||
| } |
There was a problem hiding this comment.
Throwing exception in destructor was introduced in #4
This is an indicator that something went wrong and that the code didn't release mutex properly, which means you may end up with forever locked mutex.
That's being said, I understand that this PR also introduces solution for this problem for Predis implementation if timeout is used. However other cases, lack of timeout or just different implementations, still will be affected and if I remove this condition I will again have reports "my program crashed and your library didn't release the mutex" - with this error devs are at least aware of the problem.
I propose to actually throw here error (or warning), since e.g. hhvm doesn't like exception here. And maybe, just maybe, releaseLock should return true (ok), if we release lock with timeout - since timeout suggest that the lock eventually becomes available. Not saying that this is a solution, just saying my thoughts loud. Would you be ok if this code throws warning in such case? Are they still pain to handle in php?
Since you both guys are using this I will take a look into it and try to do my best to include this, I should have some time during the weekend.
There was a problem hiding this comment.
I did a code review on the pull request from @antriver today, and here are what I think:
-
Changes in class \NinjaMutex\Lock\PredisRedisLock do address the edge case in a proper way when locks are not correctly released automatically.
-
When a lock is not released automatically as expected, the PredisRedisLock class can release the lock properly once the lock is expired. Because of that, changes in method _NinjaMutex\Mutex::_destruct() and _NinjaMutex\Lock\LockAbstract::_destruct() makes sense and make things smooth during execution (especially when handling massive locks). However, ignoring exceptions/errors silently doesn't seem to be a good practice since we lose visibility on them. To me, a better approach is to have a PSR-3 logger optionally attached to the package, log failures like that with the logger without throwing out a new exception. The external logger can be customized to throw out an exception in this case if needed. However, all these are about error reporting and error handling, which is not necessarily to be addressed and this PR.
My suggestion is to adopt changes for class \NinjaMutex\Lock\PredisRedisLock, and ignore changes in class NinjaMutex\Mutex and NinjaMutex\Lock\LockAbstract for now. Changes related to error reporting/handling can be addressed later on if needed. Thanks
| @@ -22,8 +22,8 @@ public function getLockInformation() | |||
| $hostname = gethostname(); | |||
|
|
|||
| $params = array(); | |||
| $params[] = $pid; | |||
| $params[] = $hostname; | |||
| $params['pid'] = $pid; | |||
There was a problem hiding this comment.
I had to give these keys because now that there can be different information providers the order, length, and contents of the lock values will vary. The predis lock adds an 'expiration' key to this so the structure needs to be known.
Plus it just makes sense to give these keys now that the data can be anything a provider wants to use.
| @@ -20,10 +20,9 @@ class ResolvedHostnameLockInformationProvider extends BasicLockInformationProvid | |||
| */ | |||
| public function getLockInformation() | |||
| { | |||
| $params = parent::gatherInformation(); | |||
| $params[] = gethostbyname(gethostname()); | |||
| $params = parent::getLockInformation(); | |||
There was a problem hiding this comment.
Fixed the parent call - gatherInformation does not exist.
| $params = parent::gatherInformation(); | ||
| $params[] = gethostbyname(gethostname()); | ||
| $params = parent::getLockInformation(); | ||
| $params['hostIp'] = gethostbyname(gethostname()); |
There was a problem hiding this comment.
Added a key the same as above in BasicLockInformationProvider
|
@antriver I'm ok with anything that doesn't brake tests, but travis test run is broken right now 🤷 |
antriver
force-pushed
the
predis-timeout
branch
from
e4a1b62
to
9740c6a
Compare
This adds the ability to set an expiration time on the lock when using the Predis Redis driver.
It follows the procedure recommended in the Redis docs here: https://redis.io/commands/setnx