`close` and `disconnect!` baked into `TimedStack#discard!`

[mylesmegyesi]

I'm using connection_pool for a socket client that uses the method disconnect to actually close the socket. I noticed that my sockets were not being closed properly after being discarded from the pool and I was puzzled. I found that TimedStack tries to call either close or disconnect! on the connection before discarding. There's some disadvantages to this strategy:

1) Not all cases can be covered for every possible client out there. 2) The disconnect method on my socket has the potential to hang, so I need a way to safely timeout if necessary.

After some brainstorming I came up with a few potential solutions. What do you think about adding an interface for closing connections, instead of trying to cover all the common bases in TimedStack?

pool = ConnectionPool.new { new_socket }
pool.close_with do |connection|
  connection.disconnect
end

However, this means that the interface is now a little inconsistent with itself, as the creation block is passed into the constructor and the close block has its own method. Here's some ideas that offer and more consistent interface:

# both use methods
pool = ConnectionPool.new
pool.open_with do
  new_socket
end
pool.close_with do |socket|
  socket.disconnect
end

# lifecycle strategy
pool = ConnectionPool.new do |lifecycle|
  lifecycle.open_with do
    new_socket
  end

  lifecycle.close_with do |socket|
    socket.disconnect
  end
end

# lifecycle object
class Lifecycle
  def open
    new_socket
  end

  def close(socket)
    socket.disconnect
  end
end

pool = ConnectionPool.new(Lifecycle.new)

I would be happy to submit a PR if you like any of the ideas. What do you think?

[mperham]

The contract is the network connection should provide a close method. This is the standard method in Java, Go, C, etc. connection_pool only special cases disconnect! because the redis-rb authors refused to provide a close method for whatever reason and Sidekiq is the main downstream use of connection_pool.

[mylesmegyesi]

I see. Would you consider closing connections inside ConnectionPool instead of TimedStack?

[mperham]

That doesn't seem right. Experience would lead me to think that the thing which opens the connections should also be the thing that closes the connections.

[tamird]

I believe this is moot, now that #75 is merged.