# Redis lock implementation

Considering I'm not implementing distributed lock mechanism, is this code correct and clear?

class RedisLock
class NotAcquired < StandardError; end

def initialize(redis)
@redis = redis
end

def lock(key, expiration_ms)
val = SecureRandom.random_number(100000000000000000).to_s

if @redis.set(key, val, nx: true, px: expiration_ms)
yield
unlock(key, val)
true
else
false
end
end

def lock!(*args, &block)
unless lock(*args, &block)
raise NotAcquired.new("Could not acquire lock with #{args}")
end
end

def unlock(key, val)
check_and_delete = <<-LUA
if redis.call('get', KEYS[1]) == KEYS[2] then
redis.call('del', KEYS[1])
end
LUA

@redis.eval(check_and_delete, [key, val])
end
end

RedisLock.new(Redis.current).lock!('key', 10000) { do_something }


Here's a stab at clarifying the intent without changing any of the functionality. I didn't try to run it or anything, so don't hold me to syntax.

# handles locking and unlocking redis for a particular operation
class RedisLock
# thrown when a lock cannot be acquired
class NotAcquired < StandardError; end

# since the redis key and ms are used in pretty much every method,
# might as well go ahead and pass them in the constructor.
# set defaults here if there are defaults that make sense.
# make a yardoc comment and describe what these all are.
def initialize(redis: Redis.current, nx: true, redis_key:, expiration_ms:)
@redis = redis
@redis_key = redis_key
@expiration_ms = expiration_ms
@nx = nx
end

# call it with_lock instead of lock
# to make it more apparent it accepts a block
def with_lock!
raise(NotAcquired, "Could not acquire lock with #{args}") unless lock
yield
unlock(redis_key, random_lock_number)
true # do you really need to return true here?
end

def with_lock
with_lock!
rescue NotAcquired
false
end

private

# set instance variables in the initializer, but never call them directly.

def lock
redis.set(redis_key, random_lock_number, nx: nx, px: expiration_ms)
end

# number is a little decieving here when you call .to_s at the end.
# can you use SecureRandom.uuid instead?
def random_lock_number
@random_lock_number ||= SecureRandom.random_number(100_000_000_000_000_000).to_s
end

# no need for this to be exposed publicly.
# calling eval directly is usually a bad idea.
def unlock
redis.eval(redis_check_and_delete, [redis_key, random_lock_number])
end

def redis_check_and_delete
<<-LUA
if redis.call('get', KEYS[1]) == KEYS[2] then
redis.call('del', KEYS[1])
end
LUA
end
end


And to call it:

RedisLock.new(redis_key: 'key', expiration_ms: 10000).with_lock! { do_something }

• Hi @Chris! Thanks you for the answer! Though I cannot agree with the suggested improvements. The general concern about it is that the new object cannot be re-used with another key. Moreover it even cannot be re-used with the same key. I also have minor concerns (like nx in parameters makes no sense), but this is not so important after the first point. Feb 20 '17 at 12:56
• object instantiation is free. You have no need to to re-use the single object. the Redis connection is already passed in. you've even named the class RedisLock and not something like RedisLockFactory. The major takeaway here, if you care to take away anything is that your methods are doing too much internally. If you're passing arguments into every single method, that's a code smell. also usually the ! version of a method is called by the non-bang version instead of the other way around. good luck. Feb 20 '17 at 16:42