FFM-12192 Use client Singleton / Lock eval & target map clone operati…

…on (#48)

lib/ff/ruby/server/sdk/api/cf_client.rb

@@ -1,56 +1,23 @@
-
+require 'singleton'
 require_relative "../../generated/lib/openapi_client"
 require_relative "../common/closeable"
 require_relative "inner_client"
 
 class CfClient < Closeable
-
-  # Static:
-  class << self
-
-    @@instance = CfClient.new
-
-    def instance
-
-      @@instance
+  include Singleton
+
+  def init(api_key, config, connector = nil)
+    # Only initialize if @client is nil to avoid reinitialization
+    unless @client
+      @config = config || ConfigBuilder.new.build
+      @client = InnerClient.new(api_key, @config, connector)
+      @config.logger.debug "Client initialized with API key: #{api_key}"
     end
+    @client
   end
 
-  # Static - End
-
-  def initialize(api_key = nil, config = nil, connector = nil)
-
-    if config == nil
-
-      @config = ConfigBuilder.new.build
-    else
-
-      @config = config
-    end
-
-    @client = InnerClient.new(api_key, config, connector)
-
-    @config.logger.debug "Client (1): " + @client.to_s
-  end
-
-  def init(api_key = nil, config = nil, connector = nil)
-
-    if @client == nil
-
-      @config = config
-
-      @client = InnerClient.new(
-
-        api_key = api_key,
-        config = config,
-        connector = connector
-      )
-
-      @config.logger.debug "Client (2): " + @client.to_s
-    end
-  end
 
-  def wait_for_initialization(timeout_ms: nil)
+def wait_for_initialization(timeout_ms: nil)
     if @client != nil
       @client.wait_for_initialization(timeout: timeout_ms)
     end

lib/ff/ruby/server/sdk/api/metrics_processor.rb

@@ -78,6 +78,8 @@ def init(connector, config, callback)
 
     @executor = Concurrent::FixedThreadPool.new(10)
 
+    # Used for locking the evalution and target metrics maps before we clone them
+    @metric_maps_mutex = Mutex.new
     @evaluation_metrics = FrequencyMap.new
     @target_metrics = Concurrent::Map.new
 
@@ -181,21 +183,27 @@ def run_one_iteration
   end
 
   def send_data_and_reset_cache(evaluation_metrics_map, target_metrics_map)
-    # Clone and clear evaluation metrics map
+
     evaluation_metrics_map_clone = Concurrent::Map.new
+    target_metrics_map_clone = Concurrent::Map.new
 
-    evaluation_metrics_map.each_pair do |key, value|
-      evaluation_metrics_map_clone[key] = value
-    end
+    # A single lock is used to synchronise access to both the evaluation and target metrics maps.
+    # While separate locks could be applied to each map individually, we want an interval's eval/target
+    # metrics to be processed in an atomic unit.
+    @metric_maps_mutex.synchronize do
+      # Clone and clear evaluation metrics map
+      evaluation_metrics_map.each_pair do |key, value|
+        evaluation_metrics_map_clone[key] = value
+      end
 
-    evaluation_metrics_map.clear
-    target_metrics_map_clone = Concurrent::Map.new
+      evaluation_metrics_map.clear
 
-    target_metrics_map.each_pair do |key, value|
-      target_metrics_map_clone[key] = value
-    end
+      target_metrics_map.each_pair do |key, value|
+        target_metrics_map_clone[key] = value
+      end
 
-    target_metrics_map.clear
+      target_metrics_map.clear
+    end
 
     @evaluation_warning_issued.make_false
     @target_warning_issued.make_false

test/ff/ruby/server/sdk/sdk_test.rb

@@ -31,38 +31,55 @@ def test_version_number
   end
 
   def test_client_singleton_inst
-
     instance = CfClient.instance
-    (0..@counter).each {
+    (0..@counter).each do
+      compare_equal = CfClient.instance
+      assert_equal(instance, compare_equal)
+    end
+  end
 
+  def test_client_singleton_init
+    instance = CfClient.instance
+    (0..@counter).each do
       compare_equal = CfClient.instance
-      compare_not_equal = CfClient.new("test")
+      assert_equal(instance, compare_equal)
+    end
+  end
 
-      refute_nil compare_equal
-      refute_nil compare_not_equal
+  def test_client_initialization
+    config = ConfigBuilder.new.build
+    connector = HarnessConnector.new(@string, config, nil)
+    api_key = "test_api_key"
 
-      assert_equal(instance, compare_equal)
-      assert(instance != compare_not_equal)
-    }
+    client = CfClient.instance
+    client.init(api_key, config, connector)
+
+    # Verify that @client is initialized and is an instance of InnerClient
+    inner_client = client.instance_variable_get(:@client)
+    refute_nil(inner_client, "InnerClient should be initialized")
+    assert_instance_of(InnerClient, inner_client, "InnerClient should be an instance of InnerClient")
   end
 
-  def test_client_constructor_inst
+  def test_client_reinitialization
+    config1 = ConfigBuilder.new.build
+    connector1 = HarnessConnector.new(@string, config1, nil)
+    api_key1 = "api_key_1"
 
-    test_string = "test"
-    config = ConfigBuilder.new.build
-    connector = HarnessConnector.new(test_string, config, nil)
+    client = CfClient.instance
+    client.init(api_key1, config1, connector1)
+
+    # Capture the initial @client instance
+    initial_inner_client = client.instance_variable_get(:@client)
 
-    instance_with_no_config = CfClient.new(test_string)
-    instance_with_config = CfClient.new(test_string, config)
-    instance_with_connector = CfClient.new(test_string, config, connector)
+    # Attempt to reinitialize with different parameters
+    config2 = ConfigBuilder.new.build
+    connector2 = HarnessConnector.new(@string, config2, nil)
+    api_key2 = "api_key_2"
 
-    refute_nil instance_with_config
-    refute_nil instance_with_connector
-    refute_nil instance_with_no_config
+    client.init(api_key2, config2, connector2)
 
-    assert(instance_with_config != instance_with_no_config)
-    assert(instance_with_connector != instance_with_no_config)
-    assert(instance_with_connector != instance_with_config)
+    # Verify that the @client instance remains the same
+    assert_same(initial_inner_client, client.instance_variable_get(:@client))
   end
 
   def test_config_constructor_inst
@@ -76,6 +93,28 @@ def test_config_constructor_inst
     assert(config != config_not_equal)
   end
 
+  def test_client_methods
+    config = ConfigBuilder.new.build
+    connector = HarnessConnector.new(@string, config, nil)
+    api_key = "test_api_key"
+
+    client = CfClient.instance
+    client.init(api_key, config, connector)
+
+    inner_client = client.instance_variable_get(:@client)
+    inner_client.stub :bool_variation, true do
+      assert_equal(true, client.bool_variation("identifier", "target", false))
+    end
+
+    inner_client.stub :string_variation, "variation" do
+      assert_equal("variation", client.string_variation("identifier", "target", "default"))
+    end
+
+    inner_client.stub :number_variation, 42 do
+      assert_equal(42, client.number_variation("identifier", "target", 0))
+    end
+  end
+
   def test_config_properties
 
     config = Config.new