Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ jobs:
- name: 'Latest released'
ruby: '3.0'
gemfile: "Gemfile"
- name: 'Rails edge'
ruby: '3.0'
gemfile: "Gemfile.edge"

name: ${{ matrix.entry.name }}

Expand Down
6 changes: 6 additions & 0 deletions Gemfile.edge
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
source 'https://rubygems.org'

gemspec

gem "rails", github: "rails/rails", branch: "main"
gem "snappy"
86 changes: 73 additions & 13 deletions lib/active_support/cache/memcached_store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -212,22 +212,82 @@ def reset #:nodoc:
end
end

protected
private

def read_entry(key, _options) # :nodoc:
handle_exceptions(return_value_on_error: nil) do
deserialize_entry(@connection.get(key))
if private_method_defined?(:read_serialized_entry)
class LocalStore < Strategy::LocalCache::LocalStore
def write_entry(_key, entry)
if entry.is_a?(Entry)
entry.dup_value!
end
super
end

def fetch_entry(key)
entry = @data.fetch(key) do
new_entry = yield
if entry.is_a?(Entry)
new_entry.dup_value!
end
@data[key] = new_entry
end
entry = entry.dup

if entry.is_a?(Entry)
entry.dup_value!
end

entry
end
end
end

def write_entry(key, entry, options) # :nodoc:
return true if read_only
method = options && options[:unless_exist] ? :add : :set
expires_in = expiration(options)
value = serialize_entry(entry, options)
handle_exceptions(return_value_on_error: false) do
@connection.send(method, key, value, expires_in)
true
module LocalCacheDup
def with_local_cache
use_temporary_local_cache(LocalStore.new) { yield }
Copy link
Copy Markdown
Member

@shioyama shioyama Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are not applied when Rails assigns LocalStore in middleware here:

https://github.com/rails/rails/blob/c903dfe618b8ed2b2c6d73b162a49fc4d478a855/activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb#L28

            LocalCacheRegistry.set_cache_for(local_cache_key, LocalStore.new)

This means that duping is broken for middleware-applied LocalCache when memcached_store is used, since the duping fixes above are not present in ActiveSupport::Cache::Strategy::LocalCache::LocalStore.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I'll need to use a decorator or something like that instead.

end
end
prepend LocalCacheDup

def read_entry(key, **options) # :nodoc:
deserialize_entry(read_serialized_entry(key, **options))
end

def read_serialized_entry(key, **)
handle_exceptions(return_value_on_error: nil) do
@connection.get(key)
end
end

def write_entry(key, entry, **options) # :nodoc:
return true if read_only

write_serialized_entry(key, serialize_entry(entry, **options), **options)
end

def write_serialized_entry(key, value, **options)
method = options && options[:unless_exist] ? :add : :set
expires_in = expiration(options)
handle_exceptions(return_value_on_error: false) do
@connection.send(method, key, value, expires_in)
true
end
end
else
def read_entry(key, _options) # :nodoc:
handle_exceptions(return_value_on_error: nil) do
deserialize_entry(@connection.get(key))
end
end

def write_entry(key, entry, options) # :nodoc:
return true if read_only
method = options && options[:unless_exist] ? :add : :set
expires_in = expiration(options)
value = serialize_entry(entry, options)
handle_exceptions(return_value_on_error: false) do
@connection.send(method, key, value, expires_in)
true
end
end
end

Expand Down
209 changes: 209 additions & 0 deletions test/support/local_cache_behavior.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@

module LocalCacheBehavior
def test_local_writes_are_persistent_on_the_remote_cache
retval = @cache.with_local_cache do
@cache.write("foo", "bar")
end
assert retval
assert_equal "bar", @cache.read("foo")
end

def test_clear_also_clears_local_cache
@cache.with_local_cache do
@cache.write("foo", "bar")
@cache.clear
assert_nil @cache.read("foo")
end

assert_nil @cache.read("foo")
end

def test_cleanup_clears_local_cache_but_not_remote_cache
begin
@cache.cleanup
rescue NotImplementedError
skip
end

@cache.with_local_cache do
@cache.write("foo", "bar")
assert_equal "bar", @cache.read("foo")

@cache.send(:bypass_local_cache) { @cache.write("foo", "baz") }
assert_equal "bar", @cache.read("foo")

@cache.cleanup
assert_equal "baz", @cache.read("foo")
end
end

def test_local_cache_of_write
@cache.with_local_cache do
@cache.write("foo", "bar")
@peek.delete("foo")
assert_equal "bar", @cache.read("foo")
end
end

def test_local_cache_of_read_returns_a_copy_of_the_entry
skip if ActiveSupport.gem_version < Gem::Version.new('6.1')

@cache.with_local_cache do
@cache.write(:foo, type: "bar")
value = @cache.read(:foo)
assert_equal("bar", value.delete(:type))
assert_equal({ type: "bar" }, @cache.read(:foo))
end
end

def test_local_cache_of_read
@cache.write("foo", "bar")
@cache.with_local_cache do
assert_equal "bar", @cache.read("foo")
end
end

def test_local_cache_of_read_nil
@cache.with_local_cache do
assert_nil @cache.read("foo")
@cache.send(:bypass_local_cache) { @cache.write "foo", "bar" }
assert_nil @cache.read("foo")
end
end

def test_local_cache_of_write_nil
@cache.with_local_cache do
assert @cache.write("foo", nil)
assert_nil @cache.read("foo")
@peek.write("foo", "bar")
assert_nil @cache.read("foo")
end
end

def test_local_cache_of_write_with_unless_exist
@cache.with_local_cache do
@cache.write("foo", "bar")
@cache.write("foo", "baz", unless_exist: true)
assert_equal @peek.read("foo"), @cache.read("foo")
end
end

def test_local_cache_of_delete
@cache.with_local_cache do
@cache.write("foo", "bar")
@cache.delete("foo")
assert_nil @cache.read("foo")
end
end

def test_local_cache_of_delete_matched
begin
@cache.delete_matched("*")
rescue NotImplementedError
skip
end

@cache.with_local_cache do
@cache.write("foo", "bar")
@cache.write("fop", "bar")
@cache.write("bar", "foo")
@cache.delete_matched("fo*")
assert_not @cache.exist?("foo")
assert_not @cache.exist?("fop")
assert_equal "foo", @cache.read("bar")
end
end

def test_local_cache_of_exist
@cache.with_local_cache do
@cache.write("foo", "bar")
@peek.delete("foo")
assert @cache.exist?("foo")
end
end

def test_local_cache_of_increment
@cache.with_local_cache do
@cache.write("foo", 1, raw: true)
@peek.write("foo", 2, raw: true)
@cache.increment("foo")
assert_equal 3, Integer(@cache.read("foo", raw: true))
end
end

def test_local_cache_of_decrement
@cache.with_local_cache do
@cache.write("foo", 1, raw: true)
@peek.write("foo", 3, raw: true)

@cache.decrement("foo")
assert_equal 2, Integer(@cache.read("foo", raw: true))
end
end

def test_local_cache_of_fetch_multi
@cache.with_local_cache do
@cache.fetch_multi("foo", "bar") { |_key| true }
@peek.delete("foo")
@peek.delete("bar")
assert_equal true, @cache.read("foo")
assert_equal true, @cache.read("bar")
end
end

def test_local_cache_of_read_multi
@cache.with_local_cache do
@cache.write("foo", "foo", raw: true)
@cache.write("bar", "bar", raw: true)
values = @cache.read_multi("foo", "bar", raw: true)
assert_equal "foo", @cache.read("foo", raw: true)
assert_equal "bar", @cache.read("bar", raw: true)
assert_equal "foo", values["foo"]
assert_equal "bar", values["bar"]
end
end

def test_initial_object_mutation_after_write
skip if ActiveSupport.gem_version < Gem::Version.new('6.1')

@cache.with_local_cache do
initial = +"bar"
@cache.write("foo", initial)
initial << "baz"
assert_equal "bar", @cache.read("foo")
end
end

def test_initial_object_mutation_after_fetch
skip if ActiveSupport.gem_version < Gem::Version.new('6.1')

@cache.with_local_cache do
initial = +"bar"
@cache.fetch("foo") { initial }
initial << "baz"
assert_equal "bar", @cache.read("foo")
assert_equal "bar", @cache.fetch("foo")
end
end

def test_local_race_condition_protection
@cache.with_local_cache do
time = Time.now
@cache.write("foo", "bar", expires_in: 60)
Time.stub(:now, time + 61) do
result = @cache.fetch("foo", race_condition_ttl: 10) do
assert_equal "bar", @cache.read("foo")
"baz"
end
assert_equal "baz", result
end
end
end

def test_local_cache_should_read_and_write_false
@cache.with_local_cache do
assert @cache.write("foo", false)
assert_equal false, @cache.read("foo")
end
end
end
1 change: 1 addition & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@
require 'active_support/test_case'

require_relative 'support/rails'
require_relative 'support/local_cache_behavior'

require 'memcached_store'
10 changes: 9 additions & 1 deletion test/test_memcached_store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
require 'logger'

class TestMemcachedStore < ActiveSupport::TestCase
include LocalCacheBehavior

setup do
@cache = ActiveSupport::Cache.lookup_store(:memcached_store, expires_in: 60, support_cas: true)
@cache.clear
@peek = ActiveSupport::Cache.lookup_store(:memcached_store, expires_in: 60, support_cas: true)

# Enable ActiveSupport notifications. Can be disabled in Rails 5.
Thread.current[:instrument_cache_store] = true
Expand Down Expand Up @@ -778,7 +781,12 @@ def test_raw_option_not_needed_on_read
end

def test_uncompress_regression
value = "bar" * ActiveSupport::Cache::Entry::DEFAULT_COMPRESS_LIMIT
limit = if defined? ActiveSupport::Cache::Entry::DEFAULT_COMPRESS_LIMIT
ActiveSupport::Cache::Entry::DEFAULT_COMPRESS_LIMIT
else
ActiveSupport::Cache::DEFAULT_COMPRESS_LIMIT
end
value = "bar" * limit
Zlib::Deflate.expects(:deflate).never
Zlib::Inflate.expects(:inflate).never

Expand Down