Redis lock implementation

Question

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 }

Chris Drappier · Accepted Answer · 2017-02-20 07:55:56Z

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 }

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. — cutalion
– cutalion, Commented Feb 20, 2017 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. — Chris Drappier
– Chris Drappier, Commented Feb 20, 2017 at 16:42

Stack Exchange Network

Redis lock implementation

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Redis lock implementation

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions