Lock.release(): reorder code so assignment to self.acquired_until is protected by the lock. #384
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, 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 inaccuracy 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.