diff --git a/README.md b/README.md index 522a80ac..5b1007e8 100644 --- a/README.md +++ b/README.md @@ -779,22 +779,11 @@ split_config = YAML.load_file(Rails.root.join('config', 'split.yml')) Split.redis = split_config[Rails.env] ``` -### Redis Caching (v4.0+) +### Redis Caching -> [!WARNING] -> The caching mechanism has known issues in multi-process environments (e.g., Puma) and multi-server deployments with load balancers (typical production environments). Cache invalidation does not propagate across processes or servers, which can lead to inconsistent experiment data being served to users. See [#735](https://github.com/splitrb/split/issues/735) for details. Use with caution in production environments. - -In some high-volume usage scenarios, Redis load can be incurred by repeated -fetches for fairly static data. Enabling caching will reduce this load. - - ```ruby -Split.configuration.cache = true -```` - -This currently caches: - - `Split::ExperimentCatalog.find` - - `Split::Experiment.start_time` - - `Split::Experiment.winner` +`Split.configuration.cache` no longer has any effect and can be removed. The cache +did not stay consistent across processes (see [#735](https://github.com/splitrb/split/issues/735)), +and Split now makes few enough Redis calls that it is no longer needed. ## Namespaces diff --git a/lib/split.rb b/lib/split.rb index 5fca8f95..c23b3330 100755 --- a/lib/split.rb +++ b/lib/split.rb @@ -7,7 +7,6 @@ require "split/algorithms/weighted_sample" require "split/algorithms/whiplash" require "split/alternative" -require "split/cache" require "split/configuration" require "split/encapsulated_helper" require "split/exceptions" @@ -69,10 +68,6 @@ def configure self.configuration ||= Configuration.new yield(configuration) end - - def cache(namespace, key, &block) - Split::Cache.fetch(namespace, key, &block) - end end # Check to see if being run in a Rails application. If so, wait until before_initialize to run configuration so Gems that create ENV variables have the chance to initialize first. diff --git a/lib/split/cache.rb b/lib/split/cache.rb deleted file mode 100644 index f570af7f..00000000 --- a/lib/split/cache.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module Split - class Cache - def self.clear - @cache = nil - end - - def self.fetch(namespace, key) - return yield unless Split.configuration.cache - - @cache ||= {} - @cache[namespace] ||= {} - - value = @cache[namespace][key] - return value if value - - @cache[namespace][key] = yield - end - - def self.clear_key(key) - @cache&.keys&.each do |namespace| - @cache[namespace]&.delete(key) - end - end - end -end diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index d80a1124..4566ef6e 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -17,11 +17,9 @@ class Experiment } def self.find(name) - Split.cache(:experiments, name) do - experiment = Experiment.new(name) - experiment.load_from_redis - experiment unless experiment.alternatives.empty? - end + experiment = Experiment.new(name) + experiment.load_from_redis + experiment unless experiment.alternatives.empty? end def self.find_many(names) @@ -149,13 +147,11 @@ def alternatives=(alts) end def winner - Split.cache(:experiment_winner, name) do - experiment_winner = redis.hget(:experiment_winner, name) - if experiment_winner - Split::Alternative.new(experiment_winner, name) - else - nil - end + experiment_winner = redis.hget(:experiment_winner, name) + if experiment_winner + Split::Alternative.new(experiment_winner, name) + else + nil end end @@ -181,7 +177,6 @@ def control def reset_winner redis.hdel(:experiment_winner, name) @has_winner = false - Split::Cache.clear_key(@name) end def start @@ -191,9 +186,7 @@ def start def start_time return @start_time if defined?(@start_time) - @start_time = Split.cache(:experiment_start_times, @name) do - parse_start_time(redis.hget(:experiment_start_times, @name)) - end + @start_time = parse_start_time(redis.hget(:experiment_start_times, @name)) end def prefetch_trial_state! @@ -258,7 +251,6 @@ def resettable? def reset Split.configuration.on_before_experiment_reset.call(self) - Split::Cache.clear_key(@name) alternatives.each(&:reset) reset_winner Split.configuration.on_experiment_reset.call(self) diff --git a/spec/cache_spec.rb b/spec/cache_spec.rb deleted file mode 100644 index 48b6cd46..00000000 --- a/spec/cache_spec.rb +++ /dev/null @@ -1,84 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -describe Split::Cache do - let(:namespace) { :test_namespace } - let(:key) { :test_key } - let(:now) { 1606189017 } - - before { allow(Time).to receive(:now).and_return(now) } - - describe "clear" do - before { Split.configuration.cache = true } - - it "clears the cache" do - expect(Time).to receive(:now).and_return(now).exactly(2).times - Split::Cache.fetch(namespace, key) { Time.now } - Split::Cache.clear - Split::Cache.fetch(namespace, key) { Time.now } - end - end - - describe "clear_key" do - before { Split.configuration.cache = true } - - it "clears the cache" do - expect(Time).to receive(:now).and_return(now).exactly(3).times - Split::Cache.fetch(namespace, :key1) { Time.now } - Split::Cache.fetch(namespace, :key2) { Time.now } - Split::Cache.clear_key(:key1) - - Split::Cache.fetch(namespace, :key1) { Time.now } - Split::Cache.fetch(namespace, :key2) { Time.now } - end - end - - describe "fetch" do - subject { Split::Cache.fetch(namespace, key) { Time.now } } - - context "when cache disabled" do - before { Split.configuration.cache = false } - - it "returns the yield" do - expect(subject).to eql(now) - end - - it "yields every time" do - expect(Time).to receive(:now).and_return(now).exactly(2).times - Split::Cache.fetch(namespace, key) { Time.now } - Split::Cache.fetch(namespace, key) { Time.now } - end - end - - context "when cache enabled" do - before { Split.configuration.cache = true } - - it "returns the yield" do - expect(subject).to eql(now) - end - - it "yields once" do - expect(Time).to receive(:now).and_return(now).once - Split::Cache.fetch(namespace, key) { Time.now } - Split::Cache.fetch(namespace, key) { Time.now } - end - - it "honors namespace" do - expect(Split::Cache.fetch(:a, key) { :a }).to eql(:a) - expect(Split::Cache.fetch(:b, key) { :b }).to eql(:b) - - expect(Split::Cache.fetch(:a, key) { :a }).to eql(:a) - expect(Split::Cache.fetch(:b, key) { :b }).to eql(:b) - end - - it "honors key" do - expect(Split::Cache.fetch(namespace, :a) { :a }).to eql(:a) - expect(Split::Cache.fetch(namespace, :b) { :b }).to eql(:b) - - expect(Split::Cache.fetch(namespace, :a) { :a }).to eql(:a) - expect(Split::Cache.fetch(namespace, :b) { :b }).to eql(:b) - end - end - end -end diff --git a/spec/experiment_spec.rb b/spec/experiment_spec.rb index ba28c498..47037dcf 100644 --- a/spec/experiment_spec.rb +++ b/spec/experiment_spec.rb @@ -247,6 +247,17 @@ def alternative(color) it "should have no winner initially" do expect(experiment.winner).to be_nil end + + it "should see a winner changed by another process when the cache setting is on" do + Split.configuration.cache = true + experiment.save + experiment.winner = "red" + expect(experiment.winner.name).to eq("red") + + Split.redis.hset(:experiment_winner, "link_color", "blue") + + expect(experiment.winner.name).to eq("blue") + end end describe "winner=" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 2273965f..ecd5b060 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -23,7 +23,6 @@ module GlobalSharedContext Split.redis = Redis.new Split.redis.select(10) Split.redis.flushdb - Split::Cache.clear @ab_user = mock_user @params = nil end