Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConnectionPool adapter #835

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ gem 'mysql2'
gem 'pg'
gem 'cuprite'
gem 'puma'
gem 'connection_pool'

group(:guard) do
gem 'guard'
Expand Down
14 changes: 14 additions & 0 deletions lib/flipper/adapter.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,20 @@
module Flipper
# Adding a module include so we have some hooks for stuff down the road
module Adapter
OPERATIONS = [
:add,
:clear,
:disable,
:enable,
:export,
:features,
:get_all,
:get_multi,
:get,
:import,
:remove,
]

def self.included(base)
base.extend(ClassMethods)
end
Expand Down
42 changes: 42 additions & 0 deletions lib/flipper/adapters/connection_pool.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# An adapter that uses ConnectionPool to manage connections.
#
# Usage:
#
# # Reuse an existing pool to create new adapters
# pool = ConnectionPool.new(size: 5, timeout: 5) { Redis.new }
# Flipper::Adapters::ConnectionPool.new(pool) do |client|
# Flipper::Adapters::Redis.new(client)
# end
#
# # Create a new pool that returns the adapter
# Flipper::Adapters::ConnectionPool.new(size: 5, timeout: 5) do
# Flipper::Adapters::Redis.new(Redis.new)
# end
class Flipper::Adapters::ConnectionPool
include Flipper::Adapter

# Use an existing ConnectionPool to initialize and cache new adapter instances.
class Wrapper
def initialize(pool, &initializer)
@pool = pool
@initializer = initializer
@instances = {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this hang onto resources for too long in some cases, preventing them from being garbage collected? ConnectionPool has a reload capability that will release its hold on the existing resources, but this @instances hash map will retain them and then grow larger as the pool offers up new resources.

I was initially thinking that ObjectSpace::WeakKeyMap would solve the problem, but this is a funny situation in which the map keys and values both have references to the same resource.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. I just pushed 18b2a1b which makes ConnectionPool observable. I might open an issue on the uptream repo and see if a change like this would be accepted.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a good solution. The only other thing that comes to mind is an LRU cache, but I don't know if there's a solid Ruby implementation.

end

def with(&block)
@pool.with do |resource|
yield @instances[resource] ||= @initializer.call(resource)
end
end
end

def initialize(options = {}, &adapter_initializer)
@pool = options.is_a?(ConnectionPool) ? Wrapper.new(options, &adapter_initializer) : ConnectionPool.new(options, &adapter_initializer)
end

OPERATIONS.each do |method|
define_method(method) do |*args|
@pool.with { |adapter| adapter.public_send(method, *args) }
end
end
end
33 changes: 33 additions & 0 deletions spec/flipper/adapters/connection_pool_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
require "connection_pool"
require "flipper/adapters/connection_pool"
require "flipper-redis"

RSpec.describe Flipper::Adapters::ConnectionPool do
let(:redis_options) { {url: ENV['REDIS_URL']}.compact }

before do
skip_on_error(Redis::CannotConnectError, 'Redis not available') do
Redis.new(redis_options).flushdb
end
end

context "with an existing pool" do
let(:pool) do
ConnectionPool.new(size: 1, timeout: 5) { Redis.new(redis_options) }
end

subject do
described_class.new(pool) { |client| Flipper::Adapters::Redis.new(client) }
end

it_should_behave_like 'a flipper adapter'
end

context "with a new pool" do
subject do
described_class.new(size: 2) { Flipper::Adapters::Redis.new(Redis.new(redis_options)) }
end

it_should_behave_like 'a flipper adapter'
end
end