1. Michael Bayer
  2. dogpile.cache
  3. Pull requests

Pull requests

#22 Declined
Repository
wooparadog
Branch
wraps-redis-lock
Repository
zzzeek
Branch
master

Wraps redis `LuaLock` or `Lock` object into `RedisLockWrapper`

Author
  1. wooparadog
Reviewers
Description

Wraps redis lock, only exposing acquire and release.

Upper level functions should not call redis object directly. And catch RedisError while releasing a lock, because when trying to release a timeouted lock, redis will raise the following error.

Traceback (most recent call last):
  File "/Users/WooParadog/Codes/github/dogpile.cache/tests/cache/test_redis_backend.py", line 54, in test_catch_any_exceptions_when_releasing_mutex
    mutex.release()
  File "/Users/WooParadog/.virtualenvs/dogpile/lib/python2.7/site-packages/redis/lock.py", line 104, in release
    self.do_release()
  File "/Users/WooParadog/.virtualenvs/dogpile/lib/python2.7/site-packages/redis/lock.py", line 236, in do_release
    raise LockError("Cannot release a lock that's no longer owned")
LockError: Cannot release a lock that's no longer owned
  • Learn about pull requests

Comments (11)

  1. Michael Bayer repo owner

    why would you want to squash that exception? Also why not propose this upstream with pyredis where the lock is actually implemented?

    as far as not exposing other methods besides acquire/release there's no reason for that (this is Python after all).

  2. wooparadog author

    why would you want to squash that exception? Also why not propose this upstream with pyredis where the lock is actually implemented?

    I thought about proposing to pyredis, but I'm not sure whether other projects might depend on such behaviors. But in dogpile.cache, since everything is already done after successfully acquiring the lock, it shouldn't raise an exception just because the lock doesn't exist.

    as far as not exposing other methods besides acquire/release there's no reason for that (this is Python after all).

    My intention for that is to abstract the Lock object. Yeah, I know python is ducktype and all, but since Lock objects in dogpile.core already acts as an abstract obj, I thought it's best to wrap redis Lock object. Afterall, the signature of acquire used in dogpile and that of in redis is not exactly the same.

  3. Michael Bayer repo owner

    if the lock times out and goes back to being available while you're still holding onto it, that's bad. this lock is held for the whole time that the "creator" is running and if the lock just dies off, your program is going to dogpile again. it's broken. shouldn't broken behavior raise an exception?

  4. Lx Yu

    Michael Bayer I think what wooparadog meaning is, even the lock timed out, the dogpile request should return the response normally rather than drop it.

    Currently:

    'A' request in, acquire a lock with 3s time out. 5s later, it finished, but redis expire the lock in 3s, so it can't find the lock and raise an exception. In this case, request A will not have a response.

    I think in this case, request A should return the response normally though it should not write the value to cache.

  5. wooparadog author

    Michael Bayer Yeah, it's bad that lock expires before the request finishes, but raising an exception won't prevent that, but will disrupt the current call. Developers should be notified about this problem, but I don't think raising an exception might be the best term.

    -- edit:

    prevent that => prevent another program dogpiling.

  6. Michael Bayer repo owner

    'A' request in, acquire a lock with 3s time out. 5s later, it finished, but redis expire the lock in 3s, so it can't find the lock and raise an exception. In this case, request A will not have a response.

    why doesn't that mean your application is mis-configured? the timeout should be increased, no?

    I think in this case, request A should return the response normally though it should not write the value to cache.

    Ah, now that is different. Should not write the value. that's not what this PR does and that would be pretty complicated. I'm not sure if that's the "right" behavior, of course having it be an option that someone could make happen might be nice. I think that would involve rearrangement of the logic in dogpile.core.Lock though, basically catch exceptions around the call to release(), then raise some special exception with the generated value inside of it.

    from my POV, a lock that just stops being a lock midway through is catastrophic. I'm not really getting my head around that this should be some semi-normal condition that someone might want to notice in the logs one day. I'd be pretty upset about that.

    are you folks having some kind of ongoing issue where you keep setting redis lock timeouts to super-low values (why?) that's way less than expensive generation routines? I'd imagine you'd want the timeout to be miles away from any creation routines and if it were me, and a creation routine were taking so long that it passes the configured lock timeout, id want to consider that whole call to be failed.

  7. wooparadog author

    catch exceptions around the call to release(), then raise some special exception with the generated value inside of it.

    This is a good idea, I'll look into it.

  8. Michael Bayer repo owner

    Lock in dogpile.core is an entirely different kind of lock than the usual "acquire()/release()" style of lock. If we think it's misnamed, I'd rather go there. The RedisLock we are using is public API from the pyredis package, it has acquire() /release() just like any other regular lock.

  9. Michael Bayer repo owner

    OK, Python's own lock.acquire: https://docs.python.org/2/library/threading.html#threading.Lock.acquire and then pyredis: https://github.com/andymccurdy/redis-py/blob/master/redis/lock.py#L90

    Both are called acquire(), both have a named argument "blocking" in the first argument position that accepts True/False, which if False causes the lock to return True or False if the lock were acquired or not.

    IMO it seems clear that the Redis lock is seeking to be fully compatible with the traditional "acquire()" signature. There's an additional optional argument blocking_timeout which can safely be ignored.