diff --git a/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb b/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb index 5c86147f..0a0e43af 100644 --- a/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb +++ b/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb @@ -26,7 +26,7 @@ def initialize(store, store_update_sink, logger) @store = store @store_update_sink = store_update_sink @logger = logger - @monitoring_enabled = store_supports_monitoring? + @monitoring_enabled = @store.monitoring_enabled? # Thread synchronization @mutex = Mutex.new @@ -103,6 +103,11 @@ def monitoring_enabled? @monitoring_enabled end + # (see LaunchDarkly::Interfaces::FeatureStore#available?) + def available? + @store.available? + end + # # Wraps store operations with exception handling and availability tracking. # @@ -171,28 +176,6 @@ def monitoring_enabled? @logger.error { "[LDClient] Unexpected error from data store status function: #{e.message}" } end end - - # - # Determines whether the wrapped store can support enabling monitoring. - # - # The wrapped store must provide a monitoring_enabled? method, which must - # be true. But this alone is not sufficient. - # - # Because this class wraps all interactions with a provided store, it can - # technically "monitor" any store. However, monitoring also requires that - # we notify listeners when the store is available again. - # - # We determine this by checking the store's available? method, so this - # is also a requirement for monitoring support. - # - # @return [Boolean] - # - private def store_supports_monitoring? - return false unless @store.respond_to?(:monitoring_enabled?) - return false unless @store.respond_to?(:available?) - - @store.monitoring_enabled? - end end end end diff --git a/lib/ldclient-rb/impl/data_store/status_provider.rb b/lib/ldclient-rb/impl/data_store/status_provider.rb index deae9ee0..cdb73969 100644 --- a/lib/ldclient-rb/impl/data_store/status_provider.rb +++ b/lib/ldclient-rb/impl/data_store/status_provider.rb @@ -63,7 +63,6 @@ def monitoring_enabled? # private def store_supports_monitoring? return false if @store.nil? - return false unless @store.respond_to?(:monitoring_enabled?) @store.monitoring_enabled? end diff --git a/lib/ldclient-rb/impl/data_store/store.rb b/lib/ldclient-rb/impl/data_store/store.rb index 704c82c5..becf7671 100644 --- a/lib/ldclient-rb/impl/data_store/store.rb +++ b/lib/ldclient-rb/impl/data_store/store.rb @@ -97,7 +97,7 @@ def close return nil if @persistent_store.nil? begin - @persistent_store.stop if @persistent_store.respond_to?(:stop) + @persistent_store.stop rescue => e return e end diff --git a/lib/ldclient-rb/impl/store_client_wrapper.rb b/lib/ldclient-rb/impl/store_client_wrapper.rb index 7698b43f..73fd836a 100644 --- a/lib/ldclient-rb/impl/store_client_wrapper.rb +++ b/lib/ldclient-rb/impl/store_client_wrapper.rb @@ -15,7 +15,7 @@ def initialize(store, store_update_sink, logger) # @type [LaunchDarkly::Interfaces::FeatureStore] @store = store - @monitoring_enabled = does_store_support_monitoring? + @monitoring_enabled = @store.monitoring_enabled? # @type [LaunchDarkly::Impl::DataStore::UpdateSink] @store_update_sink = store_update_sink @@ -68,6 +68,10 @@ def monitoring_enabled? @monitoring_enabled end + def available? + @store.available? + end + private def wrapper() begin yield @@ -120,28 +124,6 @@ def monitoring_enabled? @logger.error("Unexpected error from data store status function: #{e}") end end - - # This methods determines whether the wrapped store can support enabling monitoring. - # - # The wrapped store must provide a monitoring_enabled method, which must - # be true. But this alone is not sufficient. - # - # Because this class wraps all interactions with a provided store, it can - # technically "monitor" any store. However, monitoring also requires that - # we notify listeners when the store is available again. - # - # We determine this by checking the store's `available?` method, so this - # is also a requirement for monitoring support. - # - # These extra checks won't be necessary once `available` becomes a part - # of the core interface requirements and this class no longer wraps every - # feature store. - private def does_store_support_monitoring? - return false unless @store.respond_to? :monitoring_enabled? - return false unless @store.respond_to? :available? - - @store.monitoring_enabled? - end end end end diff --git a/lib/ldclient-rb/in_memory_store.rb b/lib/ldclient-rb/in_memory_store.rb index 200b629c..0c6cb2d7 100644 --- a/lib/ldclient-rb/in_memory_store.rb +++ b/lib/ldclient-rb/in_memory_store.rb @@ -20,6 +20,10 @@ def monitoring_enabled? false end + def available? + true + end + def get(kind, key) @lock.with_read_lock do coll = @items[kind] diff --git a/lib/ldclient-rb/integrations/util/store_wrapper.rb b/lib/ldclient-rb/integrations/util/store_wrapper.rb index 402e59ec..bbb600aa 100644 --- a/lib/ldclient-rb/integrations/util/store_wrapper.rb +++ b/lib/ldclient-rb/integrations/util/store_wrapper.rb @@ -44,16 +44,13 @@ def initialize(core, opts) end @inited = Concurrent::AtomicBoolean.new(false) - @has_available_method = @core.respond_to? :available? end def monitoring_enabled? - @has_available_method + true end def available? - return false unless @has_available_method - @core.available? end @@ -254,6 +251,18 @@ def upsert_internal(kind, item) def initialized_internal? end + # + # Tests whether the data store operates normally. This is the same as + # {LaunchDarkly::Interfaces::FeatureStore#available?}. + # + # The wrapper reports status monitoring as enabled for all cores. When an operation + # fails, the SDK calls this method at intervals until it returns true. + # + # @return [Boolean] true if the underlying data store is reachable + # + def available? + end + # # Performs any necessary cleanup to shut down the store when the client is being shut down. # diff --git a/lib/ldclient-rb/interfaces/feature_store.rb b/lib/ldclient-rb/interfaces/feature_store.rb index f3527b0b..65f6e7b3 100644 --- a/lib/ldclient-rb/interfaces/feature_store.rb +++ b/lib/ldclient-rb/interfaces/feature_store.rb @@ -112,54 +112,47 @@ def stop end # - # WARN: This isn't a required method on a FeatureStore yet. The SDK will - # currently check if the provided store responds to this method, and if - # it does, will take appropriate action based on the documented behavior - # below. This will become required in a future major version release of - # the SDK. + # Returns true if this store supports status monitoring. # - # Returns true if this data store implementation supports status - # monitoring. + # This is normally true only for persistent data stores. A custom + # {FeatureStore} implementation can also return true. # - # This is normally only true for persistent data stores but it could also - # be true for any custom {FeatureStore} implementation. + # If this method returns true, the store must publish a status update + # when it enters an invalid state. An invalid state is a state where an + # operation has failed, or where the store knows that operations cannot + # succeed. The store must then publish another status update when it + # returns to a valid state. The SDK uses {#available?} to detect this + # return. # - # Returning true means that the store guarantees that if it ever enters - # an invalid state (that is, an operation has failed or it knows that - # operations cannot succeed at the moment), it will publish a status - # update, and will then publish another status update once it has - # returned to a valid state. + # In-memory stores must return false. # - # Custom implementations must implement `def available?` which - # synchronously checks if the store is available. Without this method, - # the SDK cannot ensure status updates will occur once the store has gone - # offline. + # The SDK returns the same value from + # {LaunchDarkly::Interfaces::DataStore::StatusProvider#monitoring_enabled?}. # - # The same value will be returned from - # {StatusProvider::monitoring_enabled?}. + # @return [Boolean] true if the store supports status monitoring # - # def monitoring_enabled? end + def monitoring_enabled? + end # - # WARN: This isn't a required method on a FeatureStore. The SDK will - # check if the provided store responds to this method, and if it does, - # will take appropriate action based on the documented behavior below. - # Usage of this method will be dropped in a future version of the SDK. + # Tests whether the data store operates normally. # - # Tests whether the data store seems to be functioning normally. + # This must not be a detailed test of different kinds of operations. Do + # the smallest possible operation that shows that the store works. For + # example, make sure that the database is reachable. # - # This should not be a detailed test of different kinds of operations, - # but just the smallest possible operation to determine whether (for - # instance) we can reach the database. + # When one of the other methods of the store raises an exception, the SDK + # assumes that the store can be unavailable. If {#monitoring_enabled?} + # returns true, the SDK then calls this method at intervals until it + # returns true. # - # Whenever one of the store's other methods throws an exception, the SDK - # will assume that it may have become unavailable (e.g. the database - # connection was lost). The SDK will then call {#available?} at intervals - # until it returns true. + # Stores that cannot become unavailable, such as in-memory stores, must + # return true. # # @return [Boolean] true if the underlying data store is reachable # - # def available? end + def available? + end end end end diff --git a/spec/impl/data_store/feature_store_client_wrapper_spec.rb b/spec/impl/data_store/feature_store_client_wrapper_spec.rb index 0a390922..94d6cf4a 100644 --- a/spec/impl/data_store/feature_store_client_wrapper_spec.rb +++ b/spec/impl/data_store/feature_store_client_wrapper_spec.rb @@ -10,9 +10,32 @@ module DataStore let(:logger) { double.as_null_object } let(:status_sink) { double(update_status: nil) } + describe "required store methods" do + [true, false].each do |enabled| + it "reports monitoring_enabled? #{enabled} from the store" do + inner = double("store", monitoring_enabled?: enabled) + expect(described_class.new(inner, status_sink, logger).monitoring_enabled?).to be enabled + end + end + + [true, false].each do |available| + it "gets available? #{available} from the store" do + inner = double("store", monitoring_enabled?: true, available?: available) + expect(described_class.new(inner, status_sink, logger).available?).to be available + end + end + + it "stops the store" do + inner = double("store", monitoring_enabled?: false) + expect(inner).to receive(:stop).once + described_class.new(inner, status_sink, logger).stop + end + end + describe "#disable_cache" do it "forwards to the underlying store when supported" do - inner = double("store", disable_cache: nil, init: nil, get: nil, all: nil, delete: nil, upsert: nil, initialized?: false, stop: nil) + inner = double("store", disable_cache: nil, init: nil, get: nil, all: nil, delete: nil, upsert: nil, initialized?: false, + stop: nil, monitoring_enabled?: false, available?: true) expect(inner).to receive(:disable_cache).once wrapper = described_class.new(inner, status_sink, logger) @@ -27,6 +50,8 @@ def all(_); end def delete(_, _, _); end def upsert(_, _); end def initialized?; false; end + def monitoring_enabled?; false; end + def available?; true; end def stop; end end.new diff --git a/spec/impl/data_store/store_spec.rb b/spec/impl/data_store/store_spec.rb index 546371d3..75cf6ff4 100644 --- a/spec/impl/data_store/store_spec.rb +++ b/spec/impl/data_store/store_spec.rb @@ -60,6 +60,14 @@ def disable_cache @disable_cache_called_count += 1 end + def monitoring_enabled? + true + end + + def available? + true + end + def init(all_data) @init_called_count += 1 if @should_fail @@ -130,6 +138,8 @@ def all(kind); @data[kind] || {}; end def upsert(_kind, _item); end def delete(_kind, _key, _version); end def initialized?; @init_called_count > 0; end + def monitoring_enabled?; true; end + def available?; true; end def stop; end end @@ -701,6 +711,25 @@ def changed_flags end end + describe "#close" do + it "stops the persistent store" do + persistent = StubPersistentStore.new + subject.with_persistence(persistent, true, nil) + expect(persistent).to receive(:stop).once + + expect(subject.close).to be_nil + end + + it "returns the error when the persistent store fails to stop" do + persistent = StubPersistentStore.new + subject.with_persistence(persistent, true, nil) + error = RuntimeError.new("stop failed") + allow(persistent).to receive(:stop).and_raise(error) + + expect(subject.close).to be error + end + end + describe "FDv2 cache lifecycle" do let(:flag_change) do LaunchDarkly::Interfaces::DataSystem::Change.new( diff --git a/spec/impl/store_client_wrapper_spec.rb b/spec/impl/store_client_wrapper_spec.rb index 43ba7745..3f5d2ace 100644 --- a/spec/impl/store_client_wrapper_spec.rb +++ b/spec/impl/store_client_wrapper_spec.rb @@ -25,26 +25,6 @@ module Impl end end - it "will not notify sink if wrapped store cannot come back online" do - store = double - sink = double - - allow(store).to receive(:stop) - allow(store).to receive(:monitoring_enabled?).and_return(true) - allow(store).to receive(:init).and_raise(StandardError.new('init error')) - - ensure_stop(FeatureStoreClientWrapper.new(store, sink, $null_log)) do |wrapper| - begin - wrapper.init({}) - raise "init should have raised exception" - rescue StandardError - # Ignored - end - - expect(sink).not_to receive(:update_status) - end - end - it "sink will be notified when store is back online" do event = Concurrent::Event.new statuses = [] @@ -142,6 +122,17 @@ module Impl expect(checks.value).to eq 0 end end + + describe "required store methods" do + [true, false].each do |available| + it "gets available? #{available} from the store" do + store = double(monitoring_enabled?: true, available?: available, stop: nil) + ensure_stop(FeatureStoreClientWrapper.new(store, double, $null_log)) do |wrapper| + expect(wrapper.available?).to be available + end + end + end + end end end end diff --git a/spec/in_memory_feature_store_spec.rb b/spec/in_memory_feature_store_spec.rb index 6216c2fd..d3c663b6 100644 --- a/spec/in_memory_feature_store_spec.rb +++ b/spec/in_memory_feature_store_spec.rb @@ -18,5 +18,9 @@ def create_feature_store expect(store.monitoring_enabled?).to be false end + + it "is always available" do + expect(subject.new.available?).to be true + end end end diff --git a/spec/integrations/util/store_wrapper_spec.rb b/spec/integrations/util/store_wrapper_spec.rb index a696745e..b1e0c487 100644 --- a/spec/integrations/util/store_wrapper_spec.rb +++ b/spec/integrations/util/store_wrapper_spec.rb @@ -7,7 +7,7 @@ module Integrations THINGS = { namespace: "things" } - it "monitoring enabled if available is defined" do + it "reports monitoring enabled and gets availability from the core" do [true, false].each do |expected| core = double allow(core).to receive(:available?).and_return(expected) @@ -18,12 +18,10 @@ module Integrations end end - it "available is false if core doesn't support monitoring" do + it "stops the core" do core = double - wrapper = subject.new(core, {}) - - expect(wrapper.monitoring_enabled?).to be false - expect(wrapper.available?).to be false + expect(core).to receive(:stop).once + subject.new(core, {}).stop end shared_examples "tests" do |cached| @@ -355,6 +353,13 @@ def initialized_internal? @inited_query_count = @inited_query_count + 1 @inited end + + def available? + true + end + + def stop + end end end end diff --git a/spec/mock_components.rb b/spec/mock_components.rb index c7dc8b9b..fc11d010 100644 --- a/spec/mock_components.rb +++ b/spec/mock_components.rb @@ -78,6 +78,14 @@ def init(all_data) @received_data = all_data end + def monitoring_enabled? + false + end + + def available? + true + end + def stop end end