Merge pull request #43 from leandromoreira/refactor_extend_again

Refactor extend again, use only one script.

lib/redlock/client.rb

@@ -34,7 +34,9 @@ def initialize(servers = DEFAULT_REDIS_URLS, options = {})
     # Params:
     # +resource+:: the resource (or key) string to be locked.
     # +ttl+:: The time-to-live in ms for the lock.
-    # +extend+: A lock ("lock_info") to extend.
+    # +options+:: Hash of optional parameters
+    #  * +extend+: A lock ("lock_info") to extend.
+    #  * +extend_only_if_life+: If +extend+ is given, only acquire lock if currently held
     # +block+:: an optional block to be executed; after its execution, the lock (if successfully
     # acquired) is automatically unlocked.
     def lock(resource, ttl, options = {}, &block)
@@ -86,40 +88,24 @@ class RedisInstance
       # also https://github.com/sbertrang/redis-distlock/issues/2 which proposes the value-checking
       # and @maltoe for https://github.com/leandromoreira/redlock-rb/pull/20#discussion_r38903633
       LOCK_SCRIPT = <<-eos
-        if redis.call("exists", KEYS[1]) == 0 or redis.call("get", KEYS[1]) == ARGV[1] then
+        if (redis.call("exists", KEYS[1]) == 0 and ARGV[3] == "yes") or redis.call("get", KEYS[1]) == ARGV[1] then
           return redis.call("set", KEYS[1], ARGV[1], "PX", ARGV[2])
         end
       eos
 
-      EXTEND_LIFE_SCRIPT = <<-eos
-        if redis.call("get", KEYS[1]) == ARGV[1] then
-          redis.call("expire", KEYS[1], ARGV[2])
-          return 0
-        else
-          return 1
-        end
-      eos
-
       def initialize(connection)
         if connection.respond_to?(:client)
           @redis = connection
         else
-          @redis  = Redis.new(connection)
+          @redis = Redis.new(connection)
         end
 
         load_scripts
       end
 
-      def lock(resource, val, ttl)
-        recover_from_script_flush do
-          @redis.evalsha @lock_script_sha, keys: [resource], argv: [val, ttl]
-        end
-      end
-
-      def extend(resource, val, ttl)
+      def lock(resource, val, ttl, allow_new_lock)
         recover_from_script_flush do
-          rc = @redis.evalsha @extend_life_script_sha, keys: [resource], argv: [val, ttl]
-          rc == 0
+          @redis.evalsha @lock_script_sha, keys: [resource], argv: [val, ttl, allow_new_lock]
         end
       end
 
@@ -136,7 +122,6 @@ def unlock(resource, val)
       def load_scripts
         @unlock_script_sha = @redis.script(:load, UNLOCK_SCRIPT)
         @lock_script_sha = @redis.script(:load, LOCK_SCRIPT)
-        @extend_life_script_sha = @redis.script(:load, EXTEND_LIFE_SCRIPT)
       end
 
       def recover_from_script_flush
@@ -173,11 +158,11 @@ def try_lock_instances(resource, ttl, options)
     end
 
     def lock_instances(resource, ttl, options)
-      value  = options[:extend] ? options[:extend].fetch(:value) : SecureRandom.uuid
-      method = options[:extend_life] ? :extend : :lock
+      value = options.fetch(:extend, { value: SecureRandom.uuid })[:value]
+      allow_new_lock = (options[:extend_life] || options[:extend_only_if_life]) ? 'no' : 'yes'
 
       locked, time_elapsed = timed do
-        @servers.select { |s| s.send(method, resource, value, ttl) }.size
+        @servers.select { |s| s.lock resource, value, ttl, allow_new_lock }.size
       end
 
       validity = ttl - time_elapsed - drift(ttl)

spec/client_spec.rb

@@ -43,16 +43,16 @@
         expect(@lock_info[:value]).to eq(my_lock_info[:value])
       end
 
-      context 'when extend_life flag is given' do
+      context 'when extend_only_if_life flag is given' do
         it 'does not extend a non-existent lock' do
-          @lock_info = lock_manager.lock(resource_key, ttl, extend: {value: 'hello world'}, extend_life: true)
+          @lock_info = lock_manager.lock(resource_key, ttl, extend: {value: 'hello world'}, extend_only_if_life: true)
           expect(@lock_info).to eq(false)
         end
       end
 
-      context 'when extend_life flag is not given' do
+      context 'when extend_only_if_life flag is not given' do
         it "sets the given value when trying to extend a non-existent lock" do
-          @lock_info = lock_manager.lock(resource_key, ttl, extend: {value: 'hello world'}, extend_life: false)
+          @lock_info = lock_manager.lock(resource_key, ttl, extend: {value: 'hello world'}, extend_only_if_life: false)
           expect(@lock_info).to be_lock_info_for(resource_key)
           expect(@lock_info[:value]).to eq('hello world') # really we should test what's in redis
         end