release 0.2.1

CHANGELOG.md

@@ -1,3 +1,16 @@
+## 0.2.1
+
+- Fix bug when dealing with real-world Redis error conditions.
+
+## 0.2.0
+
+- Refactor class methods into instance methods to simplify implementation.
+- Increase thread safety with Memcached implementation.
+
+## 0.1.3
+
+- Properly throw Suo::LockClientError when the connection itself fails (Memcache server not reachable, etc.)
+
 ## 0.1.2
 
 - Fix retry_timeout to properly use the full time (was being calculated incorrectly).

README.md

@@ -31,12 +31,22 @@ suo.lock("some_key") do
   @puppies.pet!
 end
 
-2.times do
-  Thread.new do
-    # second argument is the number of resources - so this will run twice
-    suo.lock("other_key", 2, timeout: 0.5) { puts "Will run twice!" }
-  end
-end
+Thread.new { suo.lock("other_key", 2) { puts "One"; sleep 2 } }
+Thread.new { suo.lock("other_key", 2) { puts "Two"; sleep 2 } }
+Thread.new { suo.lock("other_key", 2) { puts "Three" } }
+
+# will print "One" "Two", but not "Three", as there are only 2 resources
+
+# custom acquisition timeouts (time to acquire)
+suo = Suo::Client::Memcached.new(client: some_dalli_client, acquisition_timeout: 1) # in seconds
+
+# manually locking/unlocking
+suo.lock("a_key")
+foo.baz!
+suo.unlock("a_key")
+
+# custom stale lock cleanup (cleaning of dead clients)
+suo = Suo::Client::Redis.new(client: some_redis_client, stale_lock_expiration: 60*5)
 ```
 
 ## TODO

lib/suo.rb

@@ -1,2 +1,16 @@
+require "securerandom"
+require "monitor"
+
+require "dalli"
+require "dalli/cas/client"
+
+require "redis"
+
+require "msgpack"
+
 require "suo/version"
-require "suo/clients"
+
+require "suo/errors"
+require "suo/client/base"
+require "suo/client/memcached"
+require "suo/client/redis"

lib/suo/client/base.rb

@@ -2,206 +2,187 @@ module Suo
   module Client
     class Base
       DEFAULT_OPTIONS = {
-        retry_timeout: 0.1,
-        retry_delay: 0.01,
+        acquisition_timeout: 0.1,
+        acquisition_delay: 0.01,
         stale_lock_expiration: 3600
       }.freeze
 
+      attr_accessor :client
+
+      include MonitorMixin
+
       def initialize(options = {})
-        @options = self.class.merge_defaults(options)
+        fail "Client required" unless options[:client]
+        @options = DEFAULT_OPTIONS.merge(options)
+        @retry_count = (@options[:acquisition_timeout] / @options[:acquisition_delay].to_f).ceil
+        @client = @options[:client]
+        super()
       end
 
-      def lock(key, resources = 1, options = {})
-        options = self.class.merge_defaults(@options.merge(options))
-        token = self.class.lock(key, resources, options)
+      def lock(key, resources = 1)
+        token = acquire_lock(key, resources)
 
-        if token
+        if block_given? && token
           begin
-            yield if block_given?
+            yield
           ensure
-            self.class.unlock(key, token, options)
+            unlock(key, token)
           end
-
-          true
         else
-          false
+          token
         end
       end
 
       def locked?(key, resources = 1)
-        self.class.locked?(key, resources, @options)
+        locks(key).size >= resources
       end
 
-      class << self
-        def lock(key, resources = 1, options = {})
-          options = merge_defaults(options)
-          acquisition_token = nil
-          token = SecureRandom.base64(16)
+      def locks(key)
+        val, _ = get(key)
+        locks = deserialize_locks(val)
 
-          retry_with_timeout(key, options) do
-            val, cas = get(key, options)
+        locks
+      end
 
-            if val.nil?
-              set_initial(key, options)
-              next
-            end
+      def refresh(key, acquisition_token)
+        retry_with_timeout(key) do
+          val, cas = get(key)
 
-            locks = deserialize_and_clear_locks(val, options)
-
-            if locks.size < resources
-              add_lock(locks, token)
-
-              newval = serialize_locks(locks)
-
-              if set(key, newval, cas, options)
-                acquisition_token = token
-                break
-              end
-            end
+          if val.nil?
+            initial_set(key)
+            next
           end
 
-          acquisition_token
+          locks = deserialize_and_clear_locks(val)
+
+          refresh_lock(locks, acquisition_token)
+
+          break if set(key, serialize_locks(locks), cas)
         end
+      end
 
-        def locked?(key, resources = 1, options = {})
-          locks(key, options).size >= resources
+      def unlock(key, acquisition_token)
+        return unless acquisition_token
+
+        retry_with_timeout(key) do
+          val, cas = get(key)
+
+          break if val.nil?
+
+          locks = deserialize_and_clear_locks(val)
+
+          acquisition_lock = remove_lock(locks, acquisition_token)
+
+          break unless acquisition_lock
+          break if set(key, serialize_locks(locks), cas)
         end
+      rescue LockClientError => _ # rubocop:disable Lint/HandleExceptions
+        # ignore - assume success due to optimistic locking
+      end
 
-        def locks(key, options)
-          options = merge_defaults(options)
-          val, _ = get(key, options)
-          locks = deserialize_locks(val)
+      def clear(key) # rubocop:disable Lint/UnusedMethodArgument
+        fail NotImplementedError
+      end
 
-          locks
-        end
+      private
 
-        def refresh(key, acquisition_token, options = {})
-          options = merge_defaults(options)
+      def acquire_lock(key, resources = 1)
+        acquisition_token = nil
+        token = SecureRandom.base64(16)
 
-          retry_with_timeout(key, options) do
-            val, cas = get(key, options)
+        retry_with_timeout(key) do
+          val, cas = get(key)
 
-            if val.nil?
-              set_initial(key, options)
-              next
+          if val.nil?
+            initial_set(key)
+            next
+          end
+
+          locks = deserialize_and_clear_locks(val)
+
+          if locks.size < resources
+            add_lock(locks, token)
+
+            newval = serialize_locks(locks)
+
+            if set(key, newval, cas)
+              acquisition_token = token
+              break
             end
-
-            locks = deserialize_and_clear_locks(val, options)
-
-            refresh_lock(locks, acquisition_token)
-
-            break if set(key, serialize_locks(locks), cas, options)
           end
         end
 
-        def unlock(key, acquisition_token, options = {})
-          options = merge_defaults(options)
+        acquisition_token
+      end
 
-          return unless acquisition_token
+      def get(key) # rubocop:disable Lint/UnusedMethodArgument
+        fail NotImplementedError
+      end
 
-          retry_with_timeout(key, options) do
-            val, cas = get(key, options)
+      def set(key, newval, oldval) # rubocop:disable Lint/UnusedMethodArgument
+        fail NotImplementedError
+      end
 
-            break if val.nil?
+      def initial_set(key) # rubocop:disable Lint/UnusedMethodArgument
+        fail NotImplementedError
+      end
 
-            locks = deserialize_and_clear_locks(val, options)
+      def synchronize(key) # rubocop:disable Lint/UnusedMethodArgument
+        mon_synchronize { yield }
+      end
 
-            acquisition_lock = remove_lock(locks, acquisition_token)
+      def retry_with_timeout(key)
+        start = Time.now.to_f
 
-            break unless acquisition_lock
-            break if set(key, serialize_locks(locks), cas, options)
+        @retry_count.times do
+          now = Time.now.to_f
+          break if now - start > @options[:acquisition_timeout]
+
+          synchronize(key) do
+            yield
           end
-        rescue FailedToAcquireLock => _ # rubocop:disable Lint/HandleExceptions
-          # ignore - assume success due to optimistic locking
+
+          sleep(rand(@options[:acquisition_delay] * 1000).to_f / 1000)
         end
+      rescue => _
+        raise LockClientError
+      end
 
-        def clear(key, options = {}) # rubocop:disable Lint/UnusedMethodArgument
-          fail NotImplementedError
+      def serialize_locks(locks)
+        MessagePack.pack(locks.map { |time, token| [time.to_f, token] })
+      end
+
+      def deserialize_and_clear_locks(val)
+        clear_expired_locks(deserialize_locks(val))
+      end
+
+      def deserialize_locks(val)
+        unpacked = (val.nil? || val == "") ? [] : MessagePack.unpack(val)
+
+        unpacked.map do |time, token|
+          [Time.at(time), token]
         end
+      rescue EOFError => _
+        []
+      end
 
-        def merge_defaults(options = {})
-          options = self::DEFAULT_OPTIONS.merge(options)
+      def clear_expired_locks(locks)
+        expired = Time.now - @options[:stale_lock_expiration]
+        locks.reject { |time, _| time < expired }
+      end
 
-          fail "Client required" unless options[:client]
+      def add_lock(locks, token)
+        locks << [Time.now.to_f, token]
+      end
 
-          options[:retry_count] = (options[:retry_timeout] / options[:retry_delay].to_f).ceil
+      def remove_lock(locks, acquisition_token)
+        lock = locks.find { |_, token| token == acquisition_token }
+        locks.delete(lock)
+      end
 
-          options
-        end
-
-        private
-
-        def get(key, options) # rubocop:disable Lint/UnusedMethodArgument
-          fail NotImplementedError
-        end
-
-        def set(key, newval, oldval, options) # rubocop:disable Lint/UnusedMethodArgument
-          fail NotImplementedError
-        end
-
-        def set_initial(key, options) # rubocop:disable Lint/UnusedMethodArgument
-          fail NotImplementedError
-        end
-
-        def synchronize(key, options)
-          yield(key, options)
-        end
-
-        def retry_with_timeout(key, options)
-          start = Time.now.to_f
-
-          options[:retry_count].times do
-            if options[:retry_timeout]
-              now = Time.now.to_f
-              break if now - start > options[:retry_timeout]
-            end
-
-            synchronize(key, options) do
-              yield
-            end
-
-            sleep(rand(options[:retry_delay] * 1000).to_f / 1000)
-          end
-        rescue => _
-          raise FailedToAcquireLock
-        end
-
-        def serialize_locks(locks)
-          MessagePack.pack(locks.map { |time, token| [time.to_f, token] })
-        end
-
-        def deserialize_and_clear_locks(val, options)
-          clear_expired_locks(deserialize_locks(val), options)
-        end
-
-        def deserialize_locks(val)
-          unpacked = (val.nil? || val == "") ? [] : MessagePack.unpack(val)
-
-          unpacked.map do |time, token|
-            [Time.at(time), token]
-          end
-        rescue EOFError => _
-          []
-        end
-
-        def clear_expired_locks(locks, options)
-          expired = Time.now - options[:stale_lock_expiration]
-          locks.reject { |time, _| time < expired }
-        end
-
-        def add_lock(locks, token)
-          locks << [Time.now.to_f, token]
-        end
-
-        def remove_lock(locks, acquisition_token)
-          lock = locks.find { |_, token| token == acquisition_token }
-          locks.delete(lock)
-        end
-
-        def refresh_lock(locks, acquisition_token)
-          remove_lock(locks, acquisition_token)
-          add_lock(locks, token)
-        end
+      def refresh_lock(locks, acquisition_token)
+        remove_lock(locks, acquisition_token)
+        add_lock(locks, token)
       end
     end
   end

lib/suo/client/errors.rb

@@ -1,7 +0,0 @@
-module Suo
-  module Client
-    module Errors
-      class FailedToAcquireLock < StandardError; end
-    end
-  end
-end

lib/suo/client/memcached.rb

@@ -6,25 +6,22 @@ module Suo
         super
       end
 
-      class << self
-        def clear(key, options = {})
-          options = merge_defaults(options)
-          options[:client].delete(key)
-        end
+      def clear(key)
+        @client.delete(key)
+      end
 
-        private
+      private
 
-        def get(key, options)
-          options[:client].get_cas(key)
-        end
+      def get(key)
+        @client.get_cas(key)
+      end
 
-        def set(key, newval, cas, options)
-          options[:client].set_cas(key, newval, cas)
-        end
+      def set(key, newval, cas)
+        @client.set_cas(key, newval, cas)
+      end
 
-        def set_initial(key, options)
-          options[:client].set(key, "")
-        end
+      def initial_set(key)
+        @client.set(key, "")
       end
     end
   end

lib/suo/client/redis.rb

@@ -6,37 +6,34 @@ module Suo
         super
       end
 
-      class << self
-        def clear(key, options = {})
-          options = merge_defaults(options)
-          options[:client].del(key)
+      def clear(key)
+        @client.del(key)
+      end
+
+      private
+
+      def get(key)
+        [@client.get(key), nil]
+      end
+
+      def set(key, newval, _)
+        ret = @client.multi do |multi|
+          multi.set(key, newval)
         end
 
-        private
+        ret && ret[0] == "OK"
+      end
 
-        def get(key, options)
-          [options[:client].get(key), nil]
+      def synchronize(key)
+        @client.watch(key) do
+          yield
         end
+      ensure
+        @client.unwatch
+      end
 
-        def set(key, newval, _, options)
-          ret = options[:client].multi do |multi|
-            multi.set(key, newval)
-          end
-
-          ret[0] == "OK"
-        end
-
-        def synchronize(key, options)
-          options[:client].watch(key) do
-            yield
-          end
-        ensure
-          options[:client].unwatch
-        end
-
-        def set_initial(key, options)
-          options[:client].set(key, "")
-        end
+      def initial_set(key)
+        @client.set(key, "")
       end
     end
   end

lib/suo/clients.rb

@@ -1,14 +0,0 @@
-require "securerandom"
-require "monitor"
-
-require "dalli"
-require "dalli/cas/client"
-
-require "redis"
-
-require "msgpack"
-
-require "suo/client/errors"
-require "suo/client/base"
-require "suo/client/memcached"
-require "suo/client/redis"

lib/suo/errors.rb

@@ -0,0 +1,3 @@
+module Suo
+  class LockClientError < StandardError; end
+end

lib/suo/version.rb

@@ -1,3 +1,3 @@
 module Suo
-  VERSION = "0.1.2"
+  VERSION = "0.2.1"
 end

suo.gemspec

@@ -20,6 +20,8 @@ Gem::Specification.new do |spec|
   spec.test_files    = spec.files.grep(%r{^(test|spec|features)/})
   spec.require_paths = ["lib"]
 
+  spec.required_ruby_version = "~> 2.0"
+
   spec.add_dependency "dalli"
   spec.add_dependency "redis"
   spec.add_dependency "msgpack"

test/client_test.rb

@@ -3,56 +3,55 @@ require "test_helper"
 TEST_KEY = "suo_test_key".freeze
 
 module ClientTests
-  def test_requires_client
-    exception = assert_raises(RuntimeError) do
-      @klass.lock(TEST_KEY, 1)
+  def test_throws_failed_error_on_bad_client
+    assert_raises(Suo::LockClientError) do
+      client = @client.class.new(client: {})
+      client.lock(TEST_KEY, 1)
     end
-
-    assert_equal "Client required", exception.message
   end
 
-  def test_class_single_resource_locking
-    lock1 = @klass.lock(TEST_KEY, 1, client: @klass_client)
+  def test_single_resource_locking
+    lock1 = @client.lock(TEST_KEY, 1)
     refute_nil lock1
 
-    locked = @klass.locked?(TEST_KEY, 1, client: @klass_client)
+    locked = @client.locked?(TEST_KEY, 1)
     assert_equal true, locked
 
-    lock2 = @klass.lock(TEST_KEY, 1, client: @klass_client)
+    lock2 = @client.lock(TEST_KEY, 1)
     assert_nil lock2
 
-    @klass.unlock(TEST_KEY, lock1, client: @klass_client)
+    @client.unlock(TEST_KEY, lock1)
 
-    locked = @klass.locked?(TEST_KEY, 1, client: @klass_client)
+    locked = @client.locked?(TEST_KEY, 1)
 
     assert_equal false, locked
   end
 
-  def test_class_multiple_resource_locking
-    lock1 = @klass.lock(TEST_KEY, 2, client: @klass_client)
+  def test_multiple_resource_locking
+    lock1 = @client.lock(TEST_KEY, 2)
     refute_nil lock1
 
-    locked = @klass.locked?(TEST_KEY, 2, client: @klass_client)
+    locked = @client.locked?(TEST_KEY, 2)
     assert_equal false, locked
 
-    lock2 = @klass.lock(TEST_KEY, 2, client: @klass_client)
+    lock2 = @client.lock(TEST_KEY, 2)
     refute_nil lock2
 
-    locked = @klass.locked?(TEST_KEY, 2, client: @klass_client)
+    locked = @client.locked?(TEST_KEY, 2)
     assert_equal true, locked
 
-    @klass.unlock(TEST_KEY, lock1, client: @klass_client)
+    @client.unlock(TEST_KEY, lock1)
 
-    locked = @klass.locked?(TEST_KEY, 1, client: @klass_client)
+    locked = @client.locked?(TEST_KEY, 1)
     assert_equal true, locked
 
-    @klass.unlock(TEST_KEY, lock2, client: @klass_client)
+    @client.unlock(TEST_KEY, lock2)
 
-    locked = @klass.locked?(TEST_KEY, 1, client: @klass_client)
+    locked = @client.locked?(TEST_KEY, 1)
     assert_equal false, locked
   end
 
-  def test_instance_single_resource_locking
+  def test_block_single_resource_locking
     locked = false
 
     @client.lock(TEST_KEY, 1) { locked = true }
@@ -60,23 +59,47 @@ module ClientTests
     assert_equal true, locked
   end
 
-  def test_instance_unlocks_on_exception
+  def test_block_unlocks_on_exception
     assert_raises(RuntimeError) do
       @client.lock(TEST_KEY, 1) { fail "Test" }
     end
 
-    locked = @klass.locked?(TEST_KEY, 1, client: @klass_client)
+    locked = @client.locked?(TEST_KEY, 1)
     assert_equal false, locked
   end
 
-  def test_instance_multiple_resource_locking
+  def test_readme_example
+    output = Queue.new
+    threads = []
+
+    threads << Thread.new { @client.lock(TEST_KEY, 2) { output << "One"; sleep 0.5 } }
+    threads << Thread.new { @client.lock(TEST_KEY, 2) { output << "Two"; sleep 0.5 } }
+    sleep 0.1
+    threads << Thread.new { @client.lock(TEST_KEY, 2) { output << "Three" } }
+
+    threads.map(&:join)
+
+    ret = []
+
+    ret << output.pop
+    ret << output.pop
+
+    ret.sort!
+
+    assert_equal 0, output.size
+    assert_equal %w(One Two), ret
+  end
+
+  def test_block_multiple_resource_locking
     success_counter = Queue.new
     failure_counter = Queue.new
 
+    client = @client.class.new(acquisition_timeout: 0.9, client: @client.client)
+
     100.times.map do |i|
       Thread.new do
-        success = @client.lock(TEST_KEY, 50, retry_timeout: 0.5) do
-          sleep(2)
+        success = client.lock(TEST_KEY, 50) do
+          sleep(3)
           success_counter << i
         end
 
@@ -88,13 +111,15 @@ module ClientTests
     assert_equal 50, failure_counter.size
   end
 
-  def test_instance_multiple_resource_locking_longer_timeout
+  def test_block_multiple_resource_locking_longer_timeout
     success_counter = Queue.new
     failure_counter = Queue.new
 
+    client = @client.class.new(acquisition_timeout: 3, client: @client.client)
+
     100.times.map do |i|
       Thread.new do
-        success = @client.lock(TEST_KEY, 50, retry_timeout: 2) do
+        success = client.lock(TEST_KEY, 50) do
           sleep(0.5)
           success_counter << i
         end
@@ -110,12 +135,12 @@ end
 
 class TestBaseClient < Minitest::Test
   def setup
-    @klass = Suo::Client::Base
+    @client = Suo::Client::Base.new(client: {})
   end
 
   def test_not_implemented
     assert_raises(NotImplementedError) do
-      @klass.send(:get, TEST_KEY, {})
+      @client.send(:get, TEST_KEY)
     end
   end
 end
@@ -124,13 +149,12 @@ class TestMemcachedClient < Minitest::Test
   include ClientTests
 
   def setup
-    @klass = Suo::Client::Memcached
-    @client = @klass.new
-    @klass_client = Dalli::Client.new("127.0.0.1:11211")
+    @dalli = Dalli::Client.new("127.0.0.1:11211")
+    @client = Suo::Client::Memcached.new
   end
 
   def teardown
-    @klass_client.delete(TEST_KEY)
+    @dalli.delete(TEST_KEY)
   end
 end
 
@@ -138,13 +162,12 @@ class TestRedisClient < Minitest::Test
   include ClientTests
 
   def setup
-    @klass = Suo::Client::Redis
-    @client = @klass.new
-    @klass_client = Redis.new
+    @redis = Redis.new
+    @client = Suo::Client::Redis.new
   end
 
   def teardown
-    @klass_client.del(TEST_KEY)
+    @redis.del(TEST_KEY)
   end
 end
 

test/test_helper.rb

@@ -6,4 +6,3 @@ require "minitest/autorun"
 require "minitest/benchmark"
 
 ENV["SUO_TEST"] = "true"
-