bbangert / retools

Redis Tools
MIT License
138 stars 36 forks source link

Distributed lock will fail if clients aren't synchronized #18

Closed brycec closed 11 years ago

brycec commented 11 years ago

The Lock class depends on time.time() which is, incorrectly, the timestamp of the current client's OS. The consequence of this, is that when two different redis clients with different OS times try to grab the same lock, one might simply over ride the other, thinking it has already expired and then both clients will enter the critical section.

I think a solution to this would be to use the Redis TIME function to acquire the timestamp for the lock. However, this introduces additional latency in acquiring the lock and I'm not completely sure of the implications. Perhaps it could all be done in a transaction? I'm willing to open a pull request for this, if this design is sufficient.

time.time() dependency: https://github.com/bbangert/retools/blob/master/retools/lock.py#L53

Redis TIME function: http://redis.io/commands/time

bbangert commented 11 years ago

Actually, got a better way to handle this using a new feature in 2.6, lua scripting. Now the entire CAS operation can be done in Redis with a single Lua script call that conditionally sets it and updates the expiration.

bbangert commented 11 years ago

brycec: If you can give this one a spin, it should be more optimal as well.