andymccurdy/redis-py — pull request #384 by chillipino (2013-09-28 22:59:07+00:00)
Title: Lock.release(): reorder code so assignment to self.acquired_until is protected by the lock.

i found two problems in the current Lock implementation. this change fixes the first; the assignment of None to self.acquired_until in release() was not protected by the lock. when reusing the same instance of a Lock in a single process, this allowed the assignment to happen after a concurrent operation acquires the lock, stomping the correct value of acquired_until with None, and causing an exception to be thrown in release() that made it appear as if a double release was happening.

the second problem is more interesting - when using locks with timeouts, Lock.release() sometimes (~1 out of every 100 acquire/release operations for my current use case) gets a value of existing that is less than acquired_until by about 0.005. this results in the lock, and all operations waiting on it, to sit around until the timeout expires. i'm still using python 2.7.5 due to library dependencies - it looks like f == float(repr(f)) [may not be true for python 2](http://bugs.python.org/issue1580), but i haven't read through that entire thread yet.

one idea is to add an error epsilon when doing timestamp comparisons; this would affect the precision of the timeout. could someone provide motivation for why the timeout value is stored in both the redis key's value and the object instance?

as a side note, repr() outputs 6 significant figures for time.time() values close to the current date, so the lock thinks it has microsecond precision - any machine's clock that's behind the others by more than a microsecond can start acquiring locks that other machines still think they own. i did see the note in the Lock.__init__() docstring about syncing clocks, but between clock drift and imprecision during each sync, i think maintaining microsecond synchronization across redis clients is impractical (feel free to correct me).

another idea would involve changing more code, similar to this: http://www.dr-josiah.com/2012/01/creating-lock-with-redis.html

i can open a separate issue to discuss the second problem, or just roll my own Lock implementation if large-scale changes to Lock are undesirable right now.