Using ConnectionPool with redis/ohm adds nil

[dirkbolte]

I used a connection pool and had Ohm using it. Ohm uses redis.multi and requires that the same connection is used in the whole context. As it interacts with redis multiple times until multi ends, it acquires a new connection several times.
As the connection pool expects a code to only have one connection associated with a fiber at one point in time, this causes nil to be added to the connection pool (see ConnectionPool.release: @available.push(@reserved.delete(fiber.object_id)) ).

More specific:

redis = EM::Synchrony::ConnectionPool.new(size: 10) { ::Redis.new }
redis.multi do
  redis.set 1, 1
  redis.get 1
  redis.set 2, 2
end

should result in ["OK", "1", "OK"], but with the current implementation, it results in [] as all calls are executed with different connections.

Next, we have a nil element in the connection list:

redis.instance_variable_get('@available')
=> [#<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 #<Redis client v3.0.4 for redis://127.0.0.1:8093/2>,
 nil]

Basically the connection pool does not work when being used with a library that expects a set of calls to be executed on the same connection.

I tried to create a fix for that to ensure that acquires a connection only once:

def execute(async)
      f = Fiber.current
      begin
        unless (conn = @reserved[f.object_id])
          conn = acquire(f)
          acquire = true
        end
        yield conn
      ensure
        release(f) if acquire && !async
      end
end

This solves the issue, Unfortunately this approach does not work with EM::Synchrony::Multi as this seem to require to get multiple connections within the same fiber.

I wonder whether you have any idea/proposal on how to make the connection pool satisfy both needs.

[igrigorik]

I think the simplest "fix" is if Ohm returned the connection object when calling multi. Aka:

redis.multi do |conn|
  conn.set 1, 1
  conn.get 1
  conn.set 2, 2
end

I think that would solve EM::Multi concern as well. Also, an older but relevant thread: #72 (comment)

[dirkbolte]

Sorry, I didn't search upfront.
As fix I currently implemented a subclass of the connection pool implementing the code snippet above - which works fine in my case.
Next to the multi issue, the current implementation uses the method name to result in a different behavior - which breaks as soon as an interface offers a method that starts with 'a', e.g.:

redis = EM::Synchrony::ConnectionPool.new(size: 5) { ::Redis.new }
pool.append 'mytest', 'a'
NoMethodError: undefined method `callback' for 11:Fixnum
from ~/.rvm/gems/ruby-1.9.3-p392@global/gems/em-synchrony-1.0.3/lib/em-synchrony/connection_pool.rb:74:in `block in method_missing'

[igrigorik]

As a workaround for "a" issue, in the past I've simply aliased the method I want to target to a different name that doesn't clash.. Not a general solution, but a quick fix.

[dirkbolte]

Would it be an option to differentiate between an async connection pool + a pipelined version for a 2.0 release?

[igrigorik]

@dirkbolte with an increment in major version number, anything is possible.. :) ... That said, we're back to same issues raised in: #72 (comment)