From a19bd4303412165fe49beec093335615bb1f4a99 Mon Sep 17 00:00:00 2001 From: Jacob Smith Date: Mon, 25 May 2020 12:24:55 -0400 Subject: [PATCH 1/2] Only shutdown a Redis pool created by SDK --- .../impl/integrations/redis_impl.rb | 3 +++ spec/redis_feature_store_spec.rb | 23 +++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/lib/ldclient-rb/impl/integrations/redis_impl.rb b/lib/ldclient-rb/impl/integrations/redis_impl.rb index 107340f8..321d4a18 100644 --- a/lib/ldclient-rb/impl/integrations/redis_impl.rb +++ b/lib/ldclient-rb/impl/integrations/redis_impl.rb @@ -33,6 +33,7 @@ def initialize(opts) @pool = opts[:pool] || ConnectionPool.new(size: max_connections) do ::Redis.new(@redis_opts) end + @pool_owned = !opts[:pool] @prefix = opts[:prefix] || LaunchDarkly::Integrations::Redis::default_prefix @logger = opts[:logger] || Config.default_logger @test_hook = opts[:test_hook] # used for unit tests, deliberately undocumented @@ -117,6 +118,8 @@ def initialized_internal? end def stop + return unless @pool_owned + if @stopped.make_true @pool.shutdown { |redis| redis.close } end diff --git a/spec/redis_feature_store_spec.rb b/spec/redis_feature_store_spec.rb index cf69f334..e8ee905d 100644 --- a/spec/redis_feature_store_spec.rb +++ b/spec/redis_feature_store_spec.rb @@ -1,3 +1,4 @@ +require "connection_pool" require "feature_store_spec_base" require "json" require "redis" @@ -27,11 +28,11 @@ def clear_all_data describe LaunchDarkly::RedisFeatureStore do subject { LaunchDarkly::RedisFeatureStore } - + break if ENV['LD_SKIP_DATABASE_TESTS'] == '1' # These tests will all fail if there isn't a Redis instance running on the default port. - + context "real Redis with local cache" do include_examples "feature_store", method(:create_redis_store), method(:clear_all_data) end @@ -59,7 +60,7 @@ def make_concurrent_modifier_test_hook(other_client, flag, start_version, end_ve flag = { key: "foo", version: 1 } test_hook = make_concurrent_modifier_test_hook(other_client, flag, 2, 4) store = create_redis_store({ test_hook: test_hook }) - + begin store.init(LaunchDarkly::FEATURES => { flag[:key] => flag }) @@ -77,7 +78,7 @@ def make_concurrent_modifier_test_hook(other_client, flag, start_version, end_ve flag = { key: "foo", version: 1 } test_hook = make_concurrent_modifier_test_hook(other_client, flag, 3, 3) store = create_redis_store({ test_hook: test_hook }) - + begin store.init(LaunchDarkly::FEATURES => { flag[:key] => flag }) @@ -89,4 +90,18 @@ def make_concurrent_modifier_test_hook(other_client, flag, start_version, end_ve other_client.close end end + + it "doesn't shut down a Redis pool passed in options" do + unowned_pool = ConnectionPool.new(size: 1, timeout: 1) { Redis.new({ url: "redis://localhost:6379" }) } + store = create_redis_store({ pool: unowned_pool }) + + begin + store.init(LaunchDarkly::FEATURES => { }) + store.stop + + expect { unowned_pool.with {} }.not_to raise_error(ConnectionPool::PoolShuttingDownError) + ensure + unowned_pool.shutdown { |conn| conn.close } + end + end end From 6aee694c11fe456373a04c5bf35e682e8213da93 Mon Sep 17 00:00:00 2001 From: Jacob Smith Date: Tue, 26 May 2020 16:12:19 -0400 Subject: [PATCH 2/2] Make pool shutdown behavior an option --- lib/ldclient-rb/impl/integrations/redis_impl.rb | 6 +++--- lib/ldclient-rb/integrations/redis.rb | 1 + lib/ldclient-rb/redis_store.rb | 1 + spec/redis_feature_store_spec.rb | 16 +++++++++++++++- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/ldclient-rb/impl/integrations/redis_impl.rb b/lib/ldclient-rb/impl/integrations/redis_impl.rb index 321d4a18..876f4240 100644 --- a/lib/ldclient-rb/impl/integrations/redis_impl.rb +++ b/lib/ldclient-rb/impl/integrations/redis_impl.rb @@ -33,7 +33,8 @@ def initialize(opts) @pool = opts[:pool] || ConnectionPool.new(size: max_connections) do ::Redis.new(@redis_opts) end - @pool_owned = !opts[:pool] + # shutdown pool on close unless the client passed a custom pool and specified not to shutdown + @pool_shutdown_on_close = (!opts[:pool] || opts.fetch(:pool_shutdown_on_close, true)) @prefix = opts[:prefix] || LaunchDarkly::Integrations::Redis::default_prefix @logger = opts[:logger] || Config.default_logger @test_hook = opts[:test_hook] # used for unit tests, deliberately undocumented @@ -118,9 +119,8 @@ def initialized_internal? end def stop - return unless @pool_owned - if @stopped.make_true + return unless @pool_shutdown_on_close @pool.shutdown { |redis| redis.close } end end diff --git a/lib/ldclient-rb/integrations/redis.rb b/lib/ldclient-rb/integrations/redis.rb index 7e447657..396c1b35 100644 --- a/lib/ldclient-rb/integrations/redis.rb +++ b/lib/ldclient-rb/integrations/redis.rb @@ -45,6 +45,7 @@ def self.default_prefix # @option opts [Integer] :expiration (15) expiration time for the in-memory cache, in seconds; 0 for no local caching # @option opts [Integer] :capacity (1000) maximum number of items in the cache # @option opts [Object] :pool custom connection pool, if desired + # @option opts [Boolean] :pool_shutdown_on_close whether calling `close` should shutdown the custom connection pool. # @return [LaunchDarkly::Interfaces::FeatureStore] a feature store object # def self.new_feature_store(opts) diff --git a/lib/ldclient-rb/redis_store.rb b/lib/ldclient-rb/redis_store.rb index 48632411..b94e61f2 100644 --- a/lib/ldclient-rb/redis_store.rb +++ b/lib/ldclient-rb/redis_store.rb @@ -35,6 +35,7 @@ class RedisFeatureStore # @option opts [Integer] :expiration expiration time for the in-memory cache, in seconds; 0 for no local caching # @option opts [Integer] :capacity maximum number of feature flags (or related objects) to cache locally # @option opts [Object] :pool custom connection pool, if desired + # @option opts [Boolean] :pool_shutdown_on_close whether calling `close` should shutdown the custom connection pool. # def initialize(opts = {}) core = LaunchDarkly::Impl::Integrations::Redis::RedisFeatureStoreCore.new(opts) diff --git a/spec/redis_feature_store_spec.rb b/spec/redis_feature_store_spec.rb index e8ee905d..e3a179b1 100644 --- a/spec/redis_feature_store_spec.rb +++ b/spec/redis_feature_store_spec.rb @@ -91,10 +91,24 @@ def make_concurrent_modifier_test_hook(other_client, flag, start_version, end_ve end end - it "doesn't shut down a Redis pool passed in options" do + it "shuts down a custom Redis pool by default" do unowned_pool = ConnectionPool.new(size: 1, timeout: 1) { Redis.new({ url: "redis://localhost:6379" }) } store = create_redis_store({ pool: unowned_pool }) + begin + store.init(LaunchDarkly::FEATURES => { }) + store.stop + + expect { unowned_pool.with {} }.to raise_error(ConnectionPool::PoolShuttingDownError) + ensure + unowned_pool.shutdown { |conn| conn.close } + end + end + + it "doesn't shut down a custom Redis pool if pool_shutdown_on_close = false" do + unowned_pool = ConnectionPool.new(size: 1, timeout: 1) { Redis.new({ url: "redis://localhost:6379" }) } + store = create_redis_store({ pool: unowned_pool, pool_shutdown_on_close: false }) + begin store.init(LaunchDarkly::FEATURES => { }) store.stop