Fix release_stale_locks! potential deadlock

Fixes #23

README.md

@@ -219,8 +219,9 @@ Testing
 Changelog
 ---------
 
-###0.3.1 April 17, 2016
+###0.3.1 April 17, 2016 (Pending)
 - Fix `sem.lock(0)` bug (thanks eugenk!).
+- Fix `release_stale_locks!` deadlock bug (thanks mfischer-zd for the bug-report!).
 
 ###0.3.0 January 24, 2016
 - Change API to include non-blocking option for `#lock` (thanks tomclose!).

lib/redis/semaphore.rb

@@ -134,7 +134,7 @@ def generate_unique_token
  end
 
  def release_stale_locks!
- simple_mutex(:release_locks, 10) do
+ simple_expiring_mutex(:release_locks, 10) do
  @redis.hgetall(grabbed_key).each do |token, locked_at|
  timed_out_at = locked_at.to_f + @stale_client_timeout
 
@@ -147,17 +147,37 @@ def release_stale_locks!
 
  private
 
- def simple_mutex(key_name, expires = nil)
- key_name = namespaced_key(key_name) if key_name.kind_of? Symbol
- token = @redis.getset(key_name, API_VERSION)
+ def simple_expiring_mutex(key_name, expires_in)
+ # Using the locking mechanism as described in
+ # http://redis.io/commands/setnx
 
- return false unless token.nil?
- @redis.expire(key_name, expires) unless expires.nil?
+ key_name = namespaced_key(key_name)
+ cached_current_time = current_time.to_f
+ my_lock_expires_at = cached_current_time + expires_in + 1
+
+ got_lock = @redis.setnx(key_name, my_lock_expires_at)
+
+ if !got_lock
+ # Check if expired
+ other_lock_expires_at = @redis.get(key_name).to_f
+
+ if other_lock_expires_at < cached_current_time
+ old_expires_at = @redis.getset(key_name, my_lock_expires_at).to_f
+
+ # Check if another client started cleanup yet. If not,
+ # then we now have the lock.
+ got_lock = (old_expires_at == other_lock_expires_at)
+ end
+ end
+
+ return false if !got_lock
 
  begin
- yield token
+ yield
  ensure
- @redis.del(key_name)
+ # Make sure not to delete the lock in case someone else already expired
+ # our lock, with one second in between to account for some lag.
+ @redis.del(key_name) if my_lock_expires_at > (current_time.to_f - 1)
  end
  end
 

redis-semaphore.gemspec

@@ -16,6 +16,7 @@ Gem::Specification.new do |s|
  s.add_development_dependency 'rake', '< 11'
  s.add_development_dependency 'rspec', '>= 2.14'
  s.add_development_dependency 'timecop'
+ s.add_development_dependency 'pry'
 
  s.description = <<description
 Implements a distributed semaphore or mutex using Redis.

spec/semaphore_spec.rb

@@ -274,4 +274,35 @@
  end
  end
 
+ # Private method tests, do not use
+ describe "simple_expiring_mutex" do
+ let(:semaphore) { Redis::Semaphore.new(:my_semaphore, :redis => @redis) }
+
+ before do
+ semaphore.class.send(:public, :simple_expiring_mutex)
+ end
+
+ it "gracefully expires stale lock" do
+ expiration = 1
+
+ Thread.new do
+ semaphore.simple_expiring_mutex(:test, expiration) do
+ sleep 3
+ end
+ end
+
+ sleep 1.5
+
+ expect(semaphore.simple_expiring_mutex(:test, expiration)).to be_falsy
+
+ sleep expiration
+
+ it_worked = false
+ semaphore.simple_expiring_mutex(:test, expiration) do
+ it_worked = true
+ end
+
+ expect(it_worked).to be_truthy
+ end
+ end
 end

spec/spec_helper.rb

@@ -5,3 +5,8 @@
 $TESTING=true
 $:.unshift File.join(File.dirname(__FILE__), '..', 'lib')
 require 'redis/semaphore'
+
+RSpec.configure do |c|
+ c.filter_run focus: true
+ c.run_all_when_everything_filtered = true
+end