feat!: Require status and stop methods on feature stores

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

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

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

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

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]

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.
         #

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

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
 

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(

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

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