Skip to content

Commit

Permalink
[ISSUE-68] - Removed concurrency option
Browse files Browse the repository at this point in the history
It does more bad than good in real world scenarios. Performance degrades.
  • Loading branch information
jlurena committed Jul 17, 2024
1 parent 3e0d60d commit c26b44b
Show file tree
Hide file tree
Showing 6 changed files with 5 additions and 114 deletions.
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ CachedResource accepts the following options as a hash:
| `:cache_key_prefix` | A prefix to be added to the cache keys. | `nil` |
| `:collection_arguments` | The arguments that identify the principal collection request. | `[:all]` |
| `:collection_synchronize` | Use collections to generate cache entries for individuals. Update the existing cached principal collection when retrieving subsets of the principal collection or individuals. | `false` |
| `:concurrent_write` | Set to true to make concurrent writes to cache after successful API response. <br>Requires the [concurrent-ruby](https://rubygems.org/gems/concurrent-ruby) gem | `false` |
| `:logger` | The logger to which CachedResource messages should be written. | The `Rails.logger` if available, or an `ActiveSupport::Logger` |
| `:race_condition_ttl` | The race condition ttl, to prevent [dog pile effect](https://en.wikipedia.org/wiki/Cache_stampede) or [cache stampede](https://en.wikipedia.org/wiki/Cache_stampede). | `86400` |
| `:ttl_randomization_scale` | A Range from which a random value will be selected to scale the ttl. | `1..2` |
Expand Down
10 changes: 0 additions & 10 deletions lib/cached_resource/cached_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,6 @@ def cached_resource(options = {})
# and establishing the necessary methods.
def setup_cached_resource!(options)
@cached_resource = CachedResource::Configuration.new(options)
if @cached_resource.concurrent_write
begin
require "concurrent/promise"
rescue LoadError
@cached_resource.logger.error(
"`concurrent_write` option is enabled, but `concurrent-ruby` is not an installed dependency"
)
raise
end
end
send :include, CachedResource::Caching
@cached_resource
end
Expand Down
14 changes: 1 addition & 13 deletions lib/cached_resource/caching.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,7 @@ def find_with_cache(*arguments)

# Clear the cache.
def clear_cache(options = nil)
if cached_resource.concurrent_write
Concurrent::Promise.execute { cache_clear(options) }
else
cache_clear(options)
end
cache_clear(options)

true
end
Expand Down Expand Up @@ -130,14 +126,6 @@ def cache_read(key)

# Write an entry to the cache for the given key and value.
def cache_write(key, object, *arguments)
if cached_resource.concurrent_write
Concurrent::Promise.execute { _cache_write(key, object, *arguments) } && true
else
_cache_write(key, object, *arguments)
end
end

def _cache_write(key, object, *arguments)
options = arguments[1] || {}
params = options[:params]
prefix_options, query_options = split_options(params)
Expand Down
2 changes: 0 additions & 2 deletions lib/cached_resource/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,13 @@ class Configuration < OpenStruct
# :cache, default: Rails.cache or ActiveSupport::Cache::MemoryStore.new,
# :logger, default: Rails.logger or ActiveSupport::Logger.new(NilIO.new),
# :cache_collections, default: true
# :concurrent_write, default: false
def initialize(options = {})
super({
cache: defined?(Rails.cache) && Rails.cache || CACHE,
cache_collections: true,
cache_key_prefix: nil,
collection_arguments: [:all],
collection_synchronize: false,
concurrent_write: false,
enabled: true,
logger: defined?(Rails.logger) && Rails.logger || LOGGER,
race_condition_ttl: 86400,
Expand Down
25 changes: 0 additions & 25 deletions spec/cached_resource/cached_resource_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,31 +44,6 @@
expect(dummy_class.instance_variable_get(:@cached_resource)).to be_a(CachedResource::Configuration)
expect(CachedResource::Configuration).to have_received(:new).with(options)
end

context "when concurrent_write is enabled" do
before do
allow(CachedResource::Configuration).to receive(:new).and_return(
double(concurrent_write: true, logger: logger)
)
end

it "requires concurrent/promise" do
expect { dummy_class.setup_cached_resource!(concurrent_write: true) }.not_to raise_error
end

context 'When "concurrent/promise" is not installed' do
before do
allow(dummy_class).to receive(:require).with("concurrent/promise").and_raise(LoadError)
end

it "requires concurrent/promise" do
expect(logger).to receive(:error).with(
"`concurrent_write` option is enabled, but `concurrent-ruby` is not an installed dependency"
).once
expect { dummy_class.setup_cached_resource!(concurrent_write: true) }.to raise_error(LoadError)
end
end
end
end

describe ".inherited" do
Expand Down
67 changes: 4 additions & 63 deletions spec/cached_resource/caching_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ def read_from_cache(key, model = Thing)
cache: CACHE,
collection_arguments: [:all],
collection_synchronize: false,
concurrent_write: false,
enabled: true,
generate_ttl: 604800,
logger: double(:thing_logger, info: nil, error: nil),
Expand All @@ -56,7 +55,6 @@ def read_from_cache(key, model = Thing)
cache: CACHE,
collection_arguments: [:all],
collection_synchronize: false,
concurrent_write: false,
enabled: true,
generate_ttl: 604800,
logger: double(:not_the_thing_logger, info: nil, error: nil),
Expand Down Expand Up @@ -227,42 +225,6 @@ def read_from_cache(key, model = Thing)
end
end

context "when concurrency is turned on" do
before do
ActiveResource::HttpMock.respond_to do |mock|
mock.get "/things/5.json", {}, {thing: {id: 1, name: ("x" * 1_000_000)}}.to_json
end
allow(thing_cached_resource).to receive(:concurrent_write).and_return(true)
end

it "caches a response asynchronously when on" do
result = Thing.find(5)
expect(read_from_cache("thing/5")).to be_nil
expect { read_from_cache("thing/5") }.to eventually(eq(result))
end

it "clears cache concurrently" do
result = Thing.find(5)
expect { read_from_cache("thing/5") }.to eventually(eq(result))
expect { Thing.clear_cache }.to not_change { read_from_cache("thing/5") }
expect { read_from_cache("thing/5") }.to eventually(be_nil)
end
end

context "when concurrency is turned off" do
before do
ActiveResource::HttpMock.respond_to do |mock|
mock.get "/things/5.json", {}, {thing: {id: 1, name: ("x" * 1_000_000)}}.to_json
end
allow(thing_cached_resource).to receive(:concurrent_write).and_return(false)
end

it "caches a response synchronously when off" do
result = Thing.find(5)
expect(read_from_cache("thing/5")).to eq(result)
end
end

context "when cache prefix is set" do
context "cache_key_prefix is a string" do
before { allow(thing_cached_resource).to receive(:cache_key_prefix).and_return("prefix123") }
Expand Down Expand Up @@ -386,31 +348,10 @@ def read_from_cache(key, model = Thing)
end

describe ".clear_cache" do
context "when concurrent_write is false" do
it "clears the cache immediately" do
expect(thing_cached_resource.cache).to receive(:delete_matched)
expect(Thing.clear_cache).to be true
expect(thing_cached_resource.logger).to have_received(:info).with(/CLEAR/)
end
end

context "when concurrent_write is true" do
before do
allow(thing_cached_resource).to receive(:concurrent_write).and_return(true)
end

it "clears the cache asynchronously" do
promise = instance_double(Concurrent::Promise)
allow(Concurrent::Promise).to receive(:execute).and_return(promise)
expect(Concurrent::Promise).to receive(:execute)
Thing.clear_cache
end

it "logs the cache clear action" do
allow(Concurrent::Promise).to receive(:execute).and_yield
Thing.clear_cache
expect(thing_cached_resource.logger).to have_received(:info).with(/CLEAR/)
end
it "clears the cache immediately" do
expect(thing_cached_resource.cache).to receive(:delete_matched)
expect(Thing.clear_cache).to be true
expect(thing_cached_resource.logger).to have_received(:info).with(/CLEAR/)
end

context "when options are provided" do
Expand Down

0 comments on commit c26b44b

Please sign in to comment.