Deadlock when reusing redis connection

[stephankaag]

Works:

  i = 0
  threads = [1, 2].collect do
    Thread.new do
      Redis::Semaphore.new("foo", :redis => Redis.new(:db => 15)).lock do
        i += 1
      end
    end
  end
  threads.each &:join
  i.should == 2

Deadlocks:

  i = 0
  threads = [1, 2].collect do
    Thread.new do
      Redis::Semaphore.new("foo", :redis => @redis).lock do
        i += 1
      end
    end
  end
  threads.each &:join
  i.should == 2

[dv]

Interesting, good catch! I think we should not allow a single Redis client to be used in different threads like this, because I'm guessing the BLPOP command we're using (which iirc freezes the connection) is not thread-safe.

I'll think about it how to best solve this.

[felipeclopes]

I was trying to lock some of my Sidekiq tasks using redis-semaphore, but I still did not find a way to make it works, it either or goes to deadlock or don't block at all.

Is there any work around for this? I was using different connections to do the lock, but apparently it doesn't lock then.

[dv]

@felipeclopes I would suggest using different connections, it should work. Can you give an example where you don't get a lock, with different connections?

The deadlock that happens if you initialize two Semaphores with the same Redis client is unavoidable since the Redis client is blocked while locked.

[felipeclopes]

I tried to use with different connections but it didn't blocked on the .lock command. The only way to make it work, was to use the Sidekiq connection, as suggested on this blog post.

Are you suggesting a solution like this one on a StackOverflow question?

Thank you for your support.

[dv]

@felipeclopes if it didn't block when using different connections, then that's a bug in redis-semaphore, since that should definitely work (it's the reason I made it :) ). Could you give some example code that didn't work for you?

It's become obvious that this is a use-case that I should cover, so I'll think about how to make redis-semaphore thread-safe. I'll probably encapsulate the locking logic in a ruby mutex.

[ixti]

You can try my "way" of dealing with Sidekiq and redis-semaphore: http://ixti.net/development/ruby/2014/03/26/share-sidekiq-s-redis-pool-with-other-part-of-your-app.html

[stephankaag]

Looks good to me. @dv ?

[dv]

Thanks for reminding me @stephankaag, I'll find some free time this week and work through the open issues on this gem.

[kimrgrey]

Yeah, using of the same redis connection in, for example, different sidekiq workers is not good idea. It will lead to the endless lock of all threads. Think, it's better to clarify this fact in README.

[dv]

I agree, due to the blocking nature of some usecases of redis-semaphore, reusing redis clients should be discouraged. Since even adding a Ruby mutex to the blocking code in redis-semaphore would not cover all possible "abuses"*, for now I won't add code to support this use-case.

I'll add a README part that explains it and warns against it. Also, it's still possible as long as you yourself take care

*: It would only cover the use case where the redis client is only used by redis-semaphore. As soon as other code also uses it the Ruby mutex will be useless.

[jeremyhaile]

Is this issue (and the README) out-of-date? The redis-rb gem is threadsafe now and the original "failing" test case works fine for me when testing with the same redis connection.

[jeremyhaile]

Never mind...I got it to lock up now...

[atomical]

Is the solution is to open a new redis connection in each thread?

[jcavalieri]

I'm seeing deadlocks even with new connections. Is there any reason why this library couldn't use a Mutex per semaphore key? I'm experimenting with an LRU cache of mutexes.