From b06fb15461e2cb4cfcba3a68236e1bd028598196 Mon Sep 17 00:00:00 2001 From: Michael Lutsiuk Date: Sun, 15 Feb 2015 02:52:28 +0200 Subject: [PATCH 1/3] Fix deadlock when passing same connection --- lib/redis/semaphore.rb | 6 +++++- spec/semaphore_spec.rb | 19 ++++++++++++++++++- spec/spec_helper.rb | 1 + 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/lib/redis/semaphore.rb b/lib/redis/semaphore.rb index 22e1645..1aaeec2 100644 --- a/lib/redis/semaphore.rb +++ b/lib/redis/semaphore.rb @@ -19,8 +19,12 @@ def initialize(name, opts = {}) @expiration = opts.delete(:expiration) @resource_count = opts.delete(:resources) || 1 @stale_client_timeout = opts.delete(:stale_client_timeout) - @redis = opts.delete(:redis) || Redis.new(opts) @use_local_time = opts.delete(:use_local_time) + @redis = if @redis = opts.delete(:redis) + @redis.dup + else + Redis.new(opts) + end @tokens = [] end diff --git a/spec/semaphore_spec.rb b/spec/semaphore_spec.rb index 75fdbd3..dac4967 100644 --- a/spec/semaphore_spec.rb +++ b/spec/semaphore_spec.rb @@ -125,6 +125,23 @@ expect(@redis.keys.count).to eq(original_key_size) end + + it "don't enters deadlock" do + i = 0 + expect { + redis = @redis.dup + Timeout.timeout(2) do + Array.new(2).map do + Thread.new do + Redis::Semaphore.new(:sem, redis: redis).lock do + i += 1 + end + end + end.each(&:join) + end + }.not_to raise_error + expect(i).to eq(2) + end end describe "semaphore with expiration" do @@ -211,7 +228,7 @@ end it "without time support should return the same time as frozen time" do - expect(@redis).to receive(:time).and_raise(Redis::CommandError) + expect(semaphore.instance_variable_get(:@redis)).to receive(:time).and_raise(Redis::CommandError) expect(semaphore.send(:current_time)).to eq(Time.now) end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 1eff8be..05a660a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -4,4 +4,5 @@ $TESTING=true $:.unshift File.join(File.dirname(__FILE__), '..', 'lib') +require 'timeout' require 'redis/semaphore' From 54a1c663d19978a3415583339ece0d67af8d78e7 Mon Sep 17 00:00:00 2001 From: Michael Lutsiuk Date: Sun, 15 Feb 2015 02:55:41 +0200 Subject: [PATCH 2/3] Make #create! independent from calls to another methods --- lib/redis/semaphore.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/redis/semaphore.rb b/lib/redis/semaphore.rb index 1aaeec2..2cbec4f 100644 --- a/lib/redis/semaphore.rb +++ b/lib/redis/semaphore.rb @@ -29,7 +29,7 @@ def initialize(name, opts = {}) end def exists_or_create! - token = @redis.getset(exists_key, EXISTS_TOKEN) + token = @redis.get(exists_key) if token.nil? create! @@ -160,7 +160,7 @@ def simple_mutex(key_name, expires = nil) end def create! - @redis.expire(exists_key, 10) + @redis.setex(exists_key, 10, EXISTS_TOKEN) @redis.multi do @redis.del(grabbed_key) From 19d6f119910fd150fece32b5c4b29c34b3fd054a Mon Sep 17 00:00:00 2001 From: Michael Lutsiuk Date: Sun, 15 Feb 2015 02:56:51 +0200 Subject: [PATCH 3/3] Can use less code here. Avoid return in block definition --- lib/redis/semaphore.rb | 6 +----- spec/semaphore_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/redis/semaphore.rb b/lib/redis/semaphore.rb index 2cbec4f..590da03 100644 --- a/lib/redis/semaphore.rb +++ b/lib/redis/semaphore.rb @@ -94,11 +94,7 @@ def locked?(token = nil) if token @redis.hexists(grabbed_key, token) else - @tokens.each do |token| - return true if locked?(token) - end - - false + @tokens.any? { |t| locked?(t) } end end diff --git a/spec/semaphore_spec.rb b/spec/semaphore_spec.rb index dac4967..3da7b23 100644 --- a/spec/semaphore_spec.rb +++ b/spec/semaphore_spec.rb @@ -228,7 +228,8 @@ end it "without time support should return the same time as frozen time" do - expect(semaphore.instance_variable_get(:@redis)).to receive(:time).and_raise(Redis::CommandError) + expect(semaphore.instance_variable_get(:@redis)).to receive(:time) + .and_raise(Redis::CommandError) expect(semaphore.send(:current_time)).to eq(Time.now) end end