librelist archives

« back to archive

Deadlocks in redis.rb

Deadlocks in redis.rb

From:
John Firebaugh
Date:
2012-07-20 @ 19:43
I'm investigating a sidekiq deadlock and it appears that several
threads are blocked on the Redis gem's internal Monitor, with stack
traces ending as this one does:

https://gist.github.com/3152780

If I understand things correctly, since sidekiq uses a connection
pool, a Redis instance should never be shared between concurrent
threads, and thus should never block on this monitor.

Can anyone explain why this would be happening?

Re: [sidekiq] Deadlocks in redis.rb

From:
Mike Perham
Date:
2012-07-20 @ 20:29
Your understanding is correct.  No one's ever reported such a deadlock
and I have no idea what might cause it.

On Fri, Jul 20, 2012 at 12:43 PM, John Firebaugh
<john.firebaugh@gmail.com> wrote:
> I'm investigating a sidekiq deadlock and it appears that several
> threads are blocked on the Redis gem's internal Monitor, with stack
> traces ending as this one does:
>
> https://gist.github.com/3152780
>
> If I understand things correctly, since sidekiq uses a connection
> pool, a Redis instance should never be shared between concurrent
> threads, and thus should never block on this monitor.
>
> Can anyone explain why this would be happening?

Re: [sidekiq] Deadlocks in redis.rb

From:
John Firebaugh
Date:
2012-07-20 @ 22:28
On Fri, Jul 20, 2012 at 1:29 PM, Mike Perham <mperham@gmail.com> wrote:
> Your understanding is correct.  No one's ever reported such a deadlock
> and I have no idea what might cause it.

It turned out to be a misuse of Redis::Semaphore on my part:

  semaphore = Sidekiq.redis {|redis|
Redis::Semaphore.new(:my_semaphore, redis) }
  semaphore.lock do
     # ...
  end

Since Redis::Semaphore stores the passed in connection, this isn't
safe. It needs to be written:

  Sidekiq.redis do |redis|
    semaphore = Redis::Semaphore.new(:my_semaphore, redis)
    semaphore.lock do
       # ...
    end
  end

But note that this consumes a connection for the duration of the lock;
you could easily drain the connection pool this way. And in either
case, if the sidekiq process crashes or gets kill -9'd, you risk
stranding the lock.

In short, I'd not recommend trying to combine Redis::Semaphore and Sidekiq.

Re: [sidekiq] Deadlocks in redis.rb

From:
Mike Perham
Date:
2012-07-20 @ 22:33
Distributed locking in general is a terrible idea.  It's a constant
source of pain in my experience.

On Fri, Jul 20, 2012 at 3:28 PM, John Firebaugh
<john.firebaugh@gmail.com> wrote:
> On Fri, Jul 20, 2012 at 1:29 PM, Mike Perham <mperham@gmail.com> wrote:
>> Your understanding is correct.  No one's ever reported such a deadlock
>> and I have no idea what might cause it.
>
> It turned out to be a misuse of Redis::Semaphore on my part:
>
>   semaphore = Sidekiq.redis {|redis|
> Redis::Semaphore.new(:my_semaphore, redis) }
>   semaphore.lock do
>      # ...
>   end
>
> Since Redis::Semaphore stores the passed in connection, this isn't
> safe. It needs to be written:
>
>   Sidekiq.redis do |redis|
>     semaphore = Redis::Semaphore.new(:my_semaphore, redis)
>     semaphore.lock do
>        # ...
>     end
>   end
>
> But note that this consumes a connection for the duration of the lock;
> you could easily drain the connection pool this way. And in either
> case, if the sidekiq process crashes or gets kill -9'd, you risk
> stranding the lock.
>
> In short, I'd not recommend trying to combine Redis::Semaphore and Sidekiq.