mperham / connection_pool

Generic connection pooling for Ruby
MIT License
1.63k stars 143 forks source link

Extract resource management in TimedStack #55

Closed drbrain closed 10 years ago

drbrain commented 10 years ago

This makes TimedStack extendable for use by multiple connections. For multiple connections the resource managed is the number of file descriptors used instead of the number of connections to a particular service.

A subclass can use thread local variables to pass extra arguments down without altering the calls which, while clumsy, is functional.

drbrain commented 10 years ago

Example of a multi-connection pool:

require 'connection_pool/timed_stack'

class ConnectionPool::TimedStackMulti < ConnectionPool::TimedStack

  def initialize(size = 0, &block)
    super

    @enqueued = 0
    @ques = Hash.new { |h, k| h[k] = [] }
    @lru = {}
    @key = :"connection_args-#{object_id}"
  end

  def empty?
    (@created - @enqueued) >= @max
  end

  def length
    @max - @created + @enqueued
  end

  def pop connection_args, timeout = 0.5
    Thread.current[@key] = connection_args

    super timeout
  end

  def push(connection_args, obj)
    Thread.current[@key] = connection_args

    super(obj)
  end

  private

  def connection_stored? # :nodoc:
    connection_args = Thread.current[@key]

    !@ques[connection_args].empty?
  end

  def fetch_connection # :nodoc:
    connection_args = Thread.current[@key]

    @enqueued -= 1
    lru_update connection_args
    @ques[connection_args].pop
  end

  def lru_update(connection_args) # :nodoc:
    @lru.delete connection_args
    @lru[connection_args] = true
  end

  def shutdown_connections # :nodoc:
    @ques.each_key do |key|
      Thread.current[@key] = key

      super
    end
  end

  def store_connection(obj) # :nodoc:
    connection_args = Thread.current[@key]

    @ques[connection_args].push obj
    @enqueued += 1
  end

  def try_create # :nodoc:
    connection_args = Thread.current[@key]

    if @created >= @max && @enqueued >= 1
      oldest, = @lru.first
      @lru.delete oldest
      @ques[oldest].pop

      @created -= 1
    end

    if @created < @max
      @created += 1
      lru_update connection_args
      return @create_block.call(connection_args)
    end
  end

end
mperham commented 10 years ago

Looks good to me. I'll take a closer look later tonight.

mperham commented 10 years ago

Would it make your subclass simpler if the TimedStack methods took an options hash? Then you could pass the custom data as part of those options and not rely on TLVs.

drbrain commented 10 years ago

Yes. I'll add a commit with that change to this pull request

drbrain commented 10 years ago

↑ see @97f42e8

mperham commented 10 years ago

mostly good but pop is not backwards compatible. Technically I'd need to release 3.0.

On Mar 18, 2014, at 20:50, Eric Hodel notifications@github.com wrote:

↑ see @97f42e8

— Reply to this email directly or view it on GitHub.

drbrain commented 10 years ago

Ah, I did not notice 2.0 was released. I'll fix it up for API compatibility.

drbrain commented 10 years ago

Ok, please check @0ebb059 ↑

PS: I'm uncertain if you want multi-connection support in connection_pool or not. Should I submit a pull request with multi-connection support or create an extension gem?

mperham commented 10 years ago

Looks fine. Could you update any rdoc and readme to document the correct way to use #pop going forward, i.e. with just the options hash? We'll remove the explicit timeout parameter in 3.0 so I don't want anyone using it anymore.

I don't want multi-connection support due to the complexity it introduces. I'm ok with API and impl tweaks to make it easier to create multi-connection subclasses. At this point, I think you could introduce your own ConnectionPool subclass in NHP to handle multi-connections, yes?

drbrain commented 10 years ago

Could you update any rdoc […]

I'm not finding any mentions of TimedStack in documentation, it seems to be more of an internal class. Am I missing it? If not, once this is merged I can add documentation for TimedStack.

I don't want multi-connection support […]

No problem. I'll probably create a separate connection_pool-multi gem instead.

mperham commented 10 years ago

Ah, I guess TimedStack is not documented but I think some people use it so it would be good to get some decent rdoc. We'll want to make sure that ConnectionPool uses the #pop API as recommended.

Ok, sounds good.

drbrain commented 10 years ago

Do you want those changes in this pull request, or should I commit them after you merge?

mperham commented 10 years ago

Pull it all in here.

drbrain commented 10 years ago

I already switched ConnectionPool to the new TimedStack#pop API in one of the earlier commits, so all that was needed was the documentation in @a033bb6