Nsryjdtyk

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 }