Refactor extend life feature.

 * Remove Client#extend_life and #extend_life! methods,
   offer an additional extend_life option for Client#lock
   instead.
 * Pass options hash down the line to try_lock_instances,
   only there decide which script to use, etc.
 * Fix specs and testing helper.

Fixes #36.

lib/redlock/client.rb

@@ -37,9 +37,8 @@ def initialize(servers = DEFAULT_REDIS_URLS, options = {})
     # +extend+: A lock ("lock_info") to extend.
     # +block+:: an optional block to be executed; after its execution, the lock (if successfully
     # acquired) is automatically unlocked.
-    def lock(resource, ttl, options={}, &block)
-      extend = options[:extend] || nil
-      lock_info = try_lock_instances(resource, ttl, extend)
+    def lock(resource, ttl, options = {}, &block)
+      lock_info = try_lock_instances(resource, ttl, options)
 
       if block_given?
         begin
@@ -53,38 +52,6 @@ def lock(resource, ttl, options={}, &block)
       end
     end
 
-    # Extends the given lock for a given time. Returns true on success.
-    # Params:
-    # +to_extend+:: A lock ("lock_info") to extend.
-    # +ttl+:: The time-to-live in ms for the lock.
-    def extend_life(to_extend, ttl)
-      value = to_extend.fetch(:value)
-      resource = to_extend.fetch(:resource)
-
-
-      extended, time_elapsed = timed do
-        @servers.all? { |s| s.extend_life(resource, value, ttl) }
-      end
-
-      validity = ttl - time_elapsed - drift(ttl)
-
-      if extended
-        { validity: validity, resource: resource, value: value }
-      else
-        @servers.each { |s| s.unlock(resource, value) }
-        false
-      end
-    end
-
-    # Extends the given lock for a given time. Raises LockError on failure
-    # Params:
-    # +to_extend+:: A lock ("lock_info") to extend.
-    # +ttl+:: The time-to-live in ms for the lock.
-    def extend_life!(to_extend, ttl)
-      new_lock_info = self.extend_life(to_extend, ttl)
-      raise LockError, 'failed to extend lock' unless new_lock_info
-    end
-
     # Unlocks a resource.
     # Params:
     # +lock_info+:: the lock that has been acquired when you locked the resource.
@@ -124,7 +91,7 @@ class RedisInstance
         end
       eos
 
-      EXTEND_LOCK_SCRIPT = <<-eos
+      EXTEND_LIFE_SCRIPT = <<-eos
         if redis.call("get", KEYS[1]) == ARGV[1] then
           redis.call("expire", KEYS[1], ARGV[2])
           return 0
@@ -133,7 +100,6 @@ class RedisInstance
         end
       eos
 
-
       def initialize(connection)
         if connection.respond_to?(:client)
           @redis = connection
@@ -150,9 +116,9 @@ def lock(resource, val, ttl)
         end
       end
 
-      def extend_life(resource, val, ttl)
+      def extend(resource, val, ttl)
         recover_from_script_flush do
-          rc = @redis.evalsha @extend_lock_script_sha, keys: [resource], argv: [val, ttl]
+          rc = @redis.evalsha @extend_life_script_sha, keys: [resource], argv: [val, ttl]
           rc == 0
         end
       end
@@ -170,7 +136,7 @@ def unlock(resource, val)
       def load_scripts
         @unlock_script_sha = @redis.script(:load, UNLOCK_SCRIPT)
         @lock_script_sha = @redis.script(:load, LOCK_SCRIPT)
-        @extend_lock_script_sha = @redis.script(:load, EXTEND_LOCK_SCRIPT)
+        @extend_life_script_sha = @redis.script(:load, EXTEND_LIFE_SCRIPT)
       end
 
       def recover_from_script_flush
@@ -192,9 +158,11 @@ def recover_from_script_flush
       end
     end
 
-    def try_lock_instances(resource, ttl, extend)
-      @retry_count.times do
-        lock_info = lock_instances(resource, ttl, extend)
+    def try_lock_instances(resource, ttl, options)
+      tries = options[:extend] ? 1 : @retry_count
+
+      tries.times do
+        lock_info = lock_instances(resource, ttl, options)
         return lock_info if lock_info
 
         # Wait a random delay before retrying
@@ -204,11 +172,12 @@ def try_lock_instances(resource, ttl, extend)
       false
     end
 
-    def lock_instances(resource, ttl, extend)
-      value = extend ? extend.fetch(:value) : SecureRandom.uuid
+    def lock_instances(resource, ttl, options)
+      value  = options[:extend] ? options[:extend].fetch(:value) : SecureRandom.uuid
+      method = options[:extend_life] ? :extend : :lock
 
       locked, time_elapsed = timed do
-        @servers.select { |s| s.lock(resource, value, ttl) }.size
+        @servers.select { |s| s.send(method, resource, value, ttl) }.size
       end
 
       validity = ttl - time_elapsed - drift(ttl)

lib/redlock/testing.rb

@@ -4,17 +4,17 @@ class Client
 
     alias_method :try_lock_instances_without_testing, :try_lock_instances
 
-    def try_lock_instances(resource, ttl, extend)
+    def try_lock_instances(resource, ttl, options)
       if @testing_mode == :bypass
         {
           validity: ttl,
           resource: resource,
-          value: extend ? extend.fetch(:value) : SecureRandom.uuid
+          value: options[:extend] ? options[:extend].fetch(:value) : SecureRandom.uuid
         }
       elsif @testing_mode == :fail
         false
       else
-        try_lock_instances_without_testing resource, ttl, extend
+        try_lock_instances_without_testing resource, ttl, options
       end
     end
 

spec/client_spec.rb

@@ -43,10 +43,19 @@
         expect(@lock_info[:value]).to eq(my_lock_info[:value])
       end
 
-      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'})
-        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
+      context 'when extend_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)
+          expect(@lock_info).to eq(false)
+        end
+      end
+
+      context 'when extend_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)
+          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
       end
 
       it "doesn't extend somebody else's lock" do
@@ -170,40 +179,6 @@
     end
   end
 
-  describe "extend" do
-    context 'when lock is available' do
-      before { @lock_info = lock_manager.lock(resource_key, ttl) }
-      after(:each) { lock_manager.unlock(@lock_info) if @lock_info }
-
-      it 'can extend its own lock' do
-        lock_info = lock_manager.extend_life(@lock_info, ttl)
-        expect(lock_info).to be_lock_info_for(resource_key)
-      end
-
-      it "can't extend a nonexistent lock" do
-        lock_manager.unlock(@lock_info)
-        lock_info = lock_manager.extend_life(@lock_info, ttl)
-        expect(lock_info).to eq(false)
-      end
-    end
-  end
-
-  describe "extend!" do
-    context 'when lock is available' do
-      before { @lock_info = lock_manager.lock(resource_key, ttl) }
-      after(:each) { lock_manager.unlock(@lock_info) if @lock_info }
-
-      it 'can extend its own lock' do
-        expect{ lock_manager.extend_life!(@lock_info, ttl) }.to_not raise_error
-      end
-
-      it "can't extend a nonexistent lock" do
-        lock_manager.unlock(@lock_info)
-        expect{ lock_manager.extend_life!(@lock_info, ttl) }.to raise_error(Redlock::LockError)
-      end
-    end
-  end
-
   describe 'lock!' do
     context 'when lock is available' do
       it 'locks' do