Ttykuu

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.
 attr_reader :redis, :redis_key, :expiration_ms, :nx

 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