Thread safety and watch/multi/exec?

[corasaurus-hex]

I think there's a thread safety issue with watch/multi/exec w/ redis-rb. The following test demonstrates the issue.

test "thread safety with multi and watch" do
  next unless driver == :ruby || driver == :hiredis

  redis = Redis.connect(OPTIONS)
  mutex = Mutex.new
  cvar  = ConditionVariable.new

  redis.set "foo", 1

  # The watch in thread 1 will bleed over into the multi call in thread 2 and be
  # unwatched when the multi call in thread 2 ends. The change that we make to
  # foo in thread 2 should cause the multi in thread 1 to fail, but it doesn't.

  t1 = Thread.new do
    redis.watch "foo"
    mutex.synchronize{ cvar.wait(mutex) }
    redis.multi do |r|
      r.set "foo", 4
    end
  end

  t2 = Thread.new do
    redis.watch "bar"
    sleep 1
    result = redis.multi do |r|
      r.set "bar", 2
    end
    redis.set "foo", 5
    cvar.signal
  end

  t1.join
  t2.join
  assert_equal "5", redis.get("foo")
end

The way I'm forcing it with a condition variable and mutex is contrived, I'll admit, but it's just there to force the issue to be consistent.

Am I using the library wrong, or is this actually an issue? I suppose I could synchronize around the whole thing, or use a connection pool...

[corasaurus-hex]

Oh, that test was written to work in the project's test/thread_safety_test.rb, if you want to give it a shot.

[pietern]

This is indeed an issue. Because you need to hold the lock from the moment you call #watch until you finish your MULTI/EXEC I think the better solution here is to use a connection pool because this is very prone to lock contention (in part because the lock is held for a long time).

We can solve this by allowing #watch to take a block, which is essentially the same as wrapping it in a mutex.

[corasaurus-hex]

Allowing Redis#watch to take a block would be the quickest path to getting this working, but I think using a connection pool would be the best solution. Are there any plans to add a connection pool to redis-rb?

[ryanlecompte]

We've had great success with @mperham's connection_pool gem.

[corasaurus-hex]

That's 1.9-only, though, and @mperham has made it clear he doesn't want to support 1.8. The redis-rb gem (and most of our code) is in ruby 1.8 still, so this isn't an option. There is a fork of it that works with 1.8, but I'd love to see direct support in redis-rb.

[corasaurus-hex]

That's my attempt at providing a workaround for the issue, but I'd prefer to not have to think about thread-safety at all with the library, which is what a connection pool would buy us.

[pietern]

I closed #228 because it doesn't address the issue. What I was thinking about with WATCH taking a block is that it only takes a block to keep the lock, and optionally do an UNWATCH if an error is raised. Beyond that, it is up to the user what to do.