Merge pull request #39770 from Shopify/as-cache-coder

Make AS::Cache coder configurable

activesupport/lib/active_support/cache.rb

@@ -22,7 +22,7 @@ module ActiveSupport
 
     # These options mean something to all cache implementations. Individual cache
     # implementations may support additional options.
-    UNIVERSAL_OPTIONS = [:namespace, :compress, :compress_threshold, :expires_in, :race_condition_ttl]
+    UNIVERSAL_OPTIONS = [:namespace, :compress, :compress_threshold, :expires_in, :race_condition_ttl, :coder]
 
     module Strategy
       autoload :LocalCache, "active_support/cache/strategy/local_cache"
@@ -158,6 +158,8 @@ module ActiveSupport
     # threshold is configurable with the <tt>:compress_threshold</tt> option,
     # specified in bytes.
     class Store
+      DEFAULT_CODER = Marshal
+
       cattr_accessor :logger, instance_writer: true
 
       attr_reader :silence, :options
@@ -185,6 +187,7 @@ module ActiveSupport
       # namespace for the cache.
       def initialize(options = nil)
         @options = options ? options.dup : {}
+        @coder = @options.delete(:coder) { self.class::DEFAULT_CODER } || NullCoder
       end
 
       # Silences the logger.
@@ -581,6 +584,14 @@ module ActiveSupport
           raise NotImplementedError.new
         end
 
+        def serialize_entry(entry)
+          @coder.dump(entry)
+        end
+
+        def deserialize_entry(payload)
+          payload.nil? ? nil : @coder.load(payload)
+        end
+
         # Reads multiple entries from the cache implementation. Subclasses MAY
         # implement this method.
         def read_multi_entries(names, **options)
@@ -740,6 +751,18 @@ module ActiveSupport
         end
     end
 
+    module NullCoder # :nodoc:
+      class << self
+        def load(payload)
+          payload
+        end
+
+        def dump(entry)
+          entry
+        end
+      end
+    end
+
     # This class is used to represent cache entries. Cache entries have a value, an optional
     # expiration time, and an optional version. The expiration time is used to support the :race_condition_ttl option
     # on the cache. The version is used to support the :version option on the cache for rejecting
@@ -790,8 +813,8 @@ module ActiveSupport
       end
 
       # Returns the size of the cached value. This could be less than
-      # <tt>value.size</tt> if the data is compressed.
-      def size
+      # <tt>value.bytesize</tt> if the data is compressed.
+      def bytesize
         case value
         when NilClass
           0

activesupport/lib/active_support/cache/file_store.rb

@@ -74,7 +74,7 @@ module ActiveSupport
       private
         def read_entry(key, **options)
           if File.exist?(key)
-            entry = File.open(key) { |f| Marshal.load(f) }
+            entry = File.open(key) { |f| deserialize_entry(f.read) }
             entry if entry.is_a?(Cache::Entry)
           end
         rescue => e
@@ -85,7 +85,7 @@ module ActiveSupport
         def write_entry(key, entry, **options)
           return false if options[:unless_exist] && File.exist?(key)
           ensure_cache_path(File.dirname(key))
-          File.atomic_write(key, cache_path) { |f| Marshal.dump(entry, f) }
+          File.atomic_write(key, cache_path) { |f| f.write(serialize_entry(entry)) }
           true
         end
 

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -26,6 +26,8 @@ module ActiveSupport
     # MemCacheStore implements the Strategy::LocalCache strategy which implements
     # an in-memory cache inside of a block.
     class MemCacheStore < Store
+      DEFAULT_CODER = NullCoder # Dalli automatically Marshal values
+
       # Provide support for raw values in the local cache strategy.
       module LocalCacheWithRaw # :nodoc:
         private
@@ -142,7 +144,7 @@ module ActiveSupport
         # Write an entry to the cache.
         def write_entry(key, entry, **options)
           method = options[:unless_exist] ? :add : :set
-          value = options[:raw] ? entry.value.to_s : entry
+          value = options[:raw] ? entry.value.to_s : serialize_entry(entry)
           expires_in = options[:expires_in].to_i
           if options[:race_condition_ttl] && expires_in > 0 && !options[:raw]
             # Set the memcache expire a few minutes in the future to support race condition ttls on read
@@ -188,10 +190,10 @@ module ActiveSupport
           key
         end
 
-        def deserialize_entry(entry)
-          if entry
-            entry.is_a?(Entry) ? entry : Entry.new(entry, compress: false)
-          end
+        def deserialize_entry(payload)
+          entry = super
+          entry = Entry.new(entry, compress: false) if entry && !entry.is_a?(Entry)
+          entry
         end
 
         def rescue_error_with(fallback)

activesupport/lib/active_support/cache/memory_store.rb

@@ -18,6 +18,23 @@ module ActiveSupport
     #
     # MemoryStore is thread-safe.
     class MemoryStore < Store
+      module DupCoder # :nodoc:
+        class << self
+          def load(entry)
+            entry = entry.dup
+            entry.dup_value!
+            entry
+          end
+
+          def dump(entry)
+            entry.dup_value!
+            entry
+          end
+        end
+      end
+
+      DEFAULT_CODER = DupCoder
+
       def initialize(options = nil)
         options ||= {}
         super(options)
@@ -114,35 +131,34 @@ module ActiveSupport
       private
         PER_ENTRY_OVERHEAD = 240
 
-        def cached_size(key, entry)
-          key.to_s.bytesize + entry.size + PER_ENTRY_OVERHEAD
+        def cached_size(key, payload)
+          key.to_s.bytesize + payload.bytesize + PER_ENTRY_OVERHEAD
         end
 
         def read_entry(key, **options)
           entry = nil
           synchronize do
-            entry = @data.delete(key)
-            if entry
-              @data[key] = entry
-              entry = entry.dup
+            payload = @data.delete(key)
+            if payload
+              @data[key] = payload
+              entry = deserialize_entry(payload)
             end
           end
-          entry&.dup_value!
           entry
         end
 
         def write_entry(key, entry, **options)
-          entry.dup_value!
+          payload = serialize_entry(entry)
           synchronize do
             return false if options[:unless_exist] && @data.key?(key)
 
-            old_entry = @data.delete(key)
-            if old_entry
-              @cache_size -= (old_entry.size - entry.size)
+            old_payload = @data[key]
+            if old_payload
+              @cache_size -= (old_payload.bytesize - payload.bytesize)
             else
-              @cache_size += cached_size(key, entry)
+              @cache_size += cached_size(key, payload)
             end
-            @data[key] = entry
+            @data[key] = payload
             prune(@max_size * 0.75, @max_prune_time) if @cache_size > @max_size
             true
           end
@@ -150,9 +166,9 @@ module ActiveSupport
 
         def delete_entry(key, **options)
           synchronize do
-            entry = @data.delete(key)
-            @cache_size -= cached_size(key, entry) if entry
-            !!entry
+            payload = @data.delete(key)
+            @cache_size -= cached_size(key, payload) if payload
+            !!payload
           end
         end
 

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -169,7 +169,7 @@ module ActiveSupport
       # Race condition TTL is not set by default. This can be used to avoid
       # "thundering herd" cache writes when hot cache entries are expired.
       # See <tt>ActiveSupport::Cache::Store#fetch</tt> for more.
-      def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, expires_in: nil, race_condition_ttl: nil, error_handler: DEFAULT_ERROR_HANDLER, **redis_options)
+      def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, coder: DEFAULT_CODER, expires_in: nil, race_condition_ttl: nil, error_handler: DEFAULT_ERROR_HANDLER, **redis_options)
         @redis_options = redis_options
 
         @max_key_bytesize = MAX_KEY_BYTESIZE
@@ -177,7 +177,8 @@ module ActiveSupport
 
         super namespace: namespace,
           compress: compress, compress_threshold: compress_threshold,
-          expires_in: expires_in, race_condition_ttl: race_condition_ttl
+          expires_in: expires_in, race_condition_ttl: race_condition_ttl,
+          coder: coder
       end
 
       def redis
@@ -451,13 +452,11 @@ module ActiveSupport
           end
         end
 
-        def deserialize_entry(serialized_entry, raw:)
-          if serialized_entry
-            if raw
-              Entry.new(serialized_entry, compress: false)
+        def deserialize_entry(payload, raw:)
+          if payload && raw
+            Entry.new(payload, compress: false)
           else
-              Marshal.load(serialized_entry)
-            end
+            super(payload)
           end
         end
 
@@ -465,7 +464,7 @@ module ActiveSupport
           if raw
             entry.value.to_s
           else
-            Marshal.dump(entry)
+            super(entry)
           end
         end
 

activesupport/test/cache/behaviors.rb

@@ -6,6 +6,7 @@ require_relative "behaviors/cache_increment_decrement_behavior"
 require_relative "behaviors/cache_instrumentation_behavior"
 require_relative "behaviors/cache_store_behavior"
 require_relative "behaviors/cache_store_version_behavior"
+require_relative "behaviors/cache_store_coder_behavior"
 require_relative "behaviors/connection_pool_behavior"
 require_relative "behaviors/encoded_key_cache_behavior"
 require_relative "behaviors/failure_safety_behavior"

activesupport/test/cache/behaviors/cache_store_coder_behavior.rb

@@ -0,0 +1,80 @@
+# frozen_string_literal: true
+
+module CacheStoreCoderBehavior
+  class SpyCoder
+    attr_reader :dumped_entries, :loaded_entries
+
+    def initialize
+      @dumped_entries = []
+      @loaded_entries = []
+    end
+
+    def dump(entry)
+      @dumped_entries << entry
+      Marshal.dump(entry)
+    end
+
+    def load(payload)
+      entry = Marshal.load(payload)
+      @loaded_entries << entry
+      entry
+    end
+  end
+
+  def test_coder_recieve_the_entry_on_write
+    coder = SpyCoder.new
+    @store = lookup_store(coder: coder)
+    @store.write("foo", "bar")
+    assert_equal 1, coder.dumped_entries.size
+    entry = coder.dumped_entries.first
+    assert_instance_of ActiveSupport::Cache::Entry, entry
+    assert_equal "bar", entry.value
+  end
+
+  def test_coder_recieve_the_entry_on_read
+    coder = SpyCoder.new
+    @store = lookup_store(coder: coder)
+    @store.write("foo", "bar")
+    @store.read("foo")
+    assert_equal 1, coder.loaded_entries.size
+    entry = coder.loaded_entries.first
+    assert_instance_of ActiveSupport::Cache::Entry, entry
+    assert_equal "bar", entry.value
+  end
+
+  def test_coder_recieve_the_entry_on_read_multi
+    coder = SpyCoder.new
+    @store = lookup_store(coder: coder)
+    @store.write_multi({ "foo" => "bar", "egg" => "spam" })
+    @store.read_multi("foo", "egg")
+    assert_equal 2, coder.loaded_entries.size
+    entry = coder.loaded_entries.first
+    assert_instance_of ActiveSupport::Cache::Entry, entry
+    assert_equal "bar", entry.value
+
+    entry = coder.loaded_entries[1]
+    assert_instance_of ActiveSupport::Cache::Entry, entry
+    assert_equal "spam", entry.value
+  end
+
+  def test_coder_recieve_the_entry_on_write_multi
+    coder = SpyCoder.new
+    @store = lookup_store(coder: coder)
+    @store.write_multi({ "foo" => "bar", "egg" => "spam" })
+    assert_equal 2, coder.dumped_entries.size
+    entry = coder.dumped_entries.first
+    assert_instance_of ActiveSupport::Cache::Entry, entry
+    assert_equal "bar", entry.value
+
+    entry = coder.dumped_entries[1]
+    assert_instance_of ActiveSupport::Cache::Entry, entry
+    assert_equal "spam", entry.value
+  end
+
+  def test_coder_does_not_recieve_the_entry_on_read_miss
+    coder = SpyCoder.new
+    @store = lookup_store(coder: coder)
+    @store.read("foo")
+    assert_equal 0, coder.loaded_entries.size
+  end
+end

activesupport/test/cache/stores/file_store_test.rb

@@ -8,12 +8,17 @@ require "pathname"
 class FileStoreTest < ActiveSupport::TestCase
   attr_reader :cache_dir
 
+  def lookup_store(options = {})
+    cache_dir = options.delete(:cache_dir) { @cache_dir }
+    ActiveSupport::Cache.lookup_store(:file_store, cache_dir, options)
+  end
+
   def setup
     @cache_dir = Dir.mktmpdir("file-store-")
     Dir.mkdir(cache_dir) unless File.exist?(cache_dir)
-    @cache = ActiveSupport::Cache.lookup_store(:file_store, cache_dir, expires_in: 60)
-    @peek = ActiveSupport::Cache.lookup_store(:file_store, cache_dir, expires_in: 60)
-    @cache_with_pathname = ActiveSupport::Cache.lookup_store(:file_store, Pathname.new(cache_dir), expires_in: 60)
+    @cache = lookup_store(expires_in: 60)
+    @peek = lookup_store(expires_in: 60)
+    @cache_with_pathname = lookup_store(cache_dir: Pathname.new(cache_dir), expires_in: 60)
 
     @buffer = StringIO.new
     @cache.logger = ActiveSupport::Logger.new(@buffer)
@@ -26,6 +31,7 @@ class FileStoreTest < ActiveSupport::TestCase
 
   include CacheStoreBehavior
   include CacheStoreVersionBehavior
+  include CacheStoreCoderBehavior
   include LocalCacheBehavior
   include CacheDeleteMatchedBehavior
   include CacheIncrementDecrementBehavior

activesupport/test/cache/stores/mem_cache_store_test.rb

@@ -52,6 +52,7 @@ class MemCacheStoreTest < ActiveSupport::TestCase
 
   include CacheStoreBehavior
   include CacheStoreVersionBehavior
+  include CacheStoreCoderBehavior
   include LocalCacheBehavior
   include CacheIncrementDecrementBehavior
   include CacheInstrumentationBehavior

activesupport/test/cache/stores/memory_store_test.rb

@@ -6,11 +6,16 @@ require_relative "../behaviors"
 
 class MemoryStoreTest < ActiveSupport::TestCase
   def setup
-    @cache = ActiveSupport::Cache.lookup_store(:memory_store, expires_in: 60)
+    @cache = lookup_store(expires_in: 60)
+  end
+
+  def lookup_store(options = {})
+    ActiveSupport::Cache.lookup_store(:memory_store, options)
   end
 
   include CacheStoreBehavior
   include CacheStoreVersionBehavior
+  include CacheStoreCoderBehavior
   include CacheDeleteMatchedBehavior
   include CacheIncrementDecrementBehavior
   include CacheInstrumentationBehavior

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -112,11 +112,15 @@ module ActiveSupport::Cache::RedisCacheStoreTests
     setup do
       @namespace = "test-#{SecureRandom.hex}"
 
-      @cache = ActiveSupport::Cache::RedisCacheStore.new(timeout: 0.1, namespace: @namespace, expires_in: 60, driver: DRIVER)
+      @cache = lookup_store(expires_in: 60)
       # @cache.logger = Logger.new($stdout)  # For test debugging
 
       # For LocalCacheBehavior tests
-      @peek = ActiveSupport::Cache::RedisCacheStore.new(timeout: 0.1, namespace: @namespace, driver: DRIVER)
+      @peek = lookup_store(expires_in: 60)
+    end
+
+    def lookup_store(options = {})
+      ActiveSupport::Cache.lookup_store(:redis_cache_store, { timeout: 0.1, namespace: @namespace, driver: DRIVER }.merge(options))
     end
 
     teardown do
@@ -128,6 +132,7 @@ module ActiveSupport::Cache::RedisCacheStoreTests
   class RedisCacheStoreCommonBehaviorTest < StoreTest
     include CacheStoreBehavior
     include CacheStoreVersionBehavior
+    include CacheStoreCoderBehavior
     include LocalCacheBehavior
     include CacheIncrementDecrementBehavior
     include CacheInstrumentationBehavior