Fix `ActiveSupport::Cache` compression

(See previous commit for a description of the issue)

3 files changed, 60 insertions, 37 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index e067c4dfba..82c985fae2 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Fix bug where `ActiveSupport::Cache` will massively inflate the storage
+    size when compression is enabled (which is true by default). This patch
+    does not attempt to repair existing data: please manually flush the cache
+    to clear out the problematic entries.
+
+    *Godfrey Chan*
+
 *   Fix bug where `URI.unscape` would fail with mixed Unicode/escaped character input:
 
         URI.unescape("\xe3\x83\x90")  # => "バ"
diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb
index 6967c164ab..d06a4971db 100644
--- a/activesupport/lib/active_support/cache.rb
+++ b/activesupport/lib/active_support/cache.rb
@@ -712,17 +712,14 @@ module ActiveSupport
       DEFAULT_COMPRESS_LIMIT = 1.kilobyte
 
       # Creates a new cache entry for the specified value. Options supported are
-      # +:compress+, +:compress_threshold+, and +:expires_in+.
-      def initialize(value, options = {})
-        @value = value
-        if should_compress?(options)
-          compress!
-        end
-
-        @version    = options[:version]
+      # +:compress+, +:compress_threshold+, +:version+ and +:expires_in+.
+      def initialize(value, compress: true, compress_threshold: DEFAULT_COMPRESS_LIMIT, version: nil, expires_in: nil, **)
+        @value      = value
+        @version    = version
         @created_at = Time.now.to_f
-        @expires_in = options[:expires_in]
-        @expires_in = @expires_in.to_f if @expires_in
+        @expires_in = expires_in && expires_in.to_f
+
+        compress!(compress_threshold) if compress
       end
 
       def value
@@ -754,17 +751,13 @@ module ActiveSupport
       # Returns the size of the cached value. This could be less than
       # <tt>value.size</tt> if the data is compressed.
       def size
-        if defined?(@s)
-          @s
+        case value
+        when NilClass
+          0
+        when String
+          @value.bytesize
         else
-          case value
-          when NilClass
-            0
-          when String
-            @value.bytesize
-          else
-            @s = Marshal.dump(@value).bytesize
-          end
+          @s ||= Marshal.dump(@value).bytesize
         end
       end
 
@@ -781,31 +774,35 @@ module ActiveSupport
       end
 
       private
-        def should_compress?(options)
-          if @value && options.fetch(:compress, true)
-            compress_threshold = options.fetch(:compress_threshold, DEFAULT_COMPRESS_LIMIT)
-            serialized_value_size = (@value.is_a?(String) ? @value : marshaled_value).bytesize
+        def compress!(compress_threshold)
+          case @value
+          when nil, true, false, Numeric
+            uncompressed_size = 0
+          when String
+            uncompressed_size = @value.bytesize
+          else
+            serialized = Marshal.dump(@value)
+            uncompressed_size = serialized.bytesize
+          end
+
+          if uncompressed_size >= compress_threshold
+            serialized ||= Marshal.dump(@value)
+            compressed = Zlib::Deflate.deflate(serialized)
 
-            serialized_value_size >= compress_threshold
+            if compressed.bytesize < uncompressed_size
+              @value = compressed
+              @compressed = true
+            end
           end
         end
 
         def compressed?
-          defined?(@compressed) ? @compressed : false
-        end
-
-        def compress!
-          @value = Zlib::Deflate.deflate(marshaled_value)
-          @compressed = true
+          defined?(@compressed)
         end
 
         def uncompress(value)
           Marshal.load(Zlib::Inflate.inflate(value))
         end
-
-        def marshaled_value
-          @marshaled_value ||= Marshal.dump(@value)
-        end
     end
   end
 end
diff --git a/activesupport/test/cache/behaviors/cache_store_behavior.rb b/activesupport/test/cache/behaviors/cache_store_behavior.rb
index 84a3648ad0..0806665c27 100644
--- a/activesupport/test/cache/behaviors/cache_store_behavior.rb
+++ b/activesupport/test/cache/behaviors/cache_store_behavior.rb
@@ -162,7 +162,7 @@ module CacheStoreBehavior
   end
 
   def test_nil_with_compress_low_compress_threshold
-    assert_uncompressed(nil, compress: true, compress_threshold: 2)
+    assert_uncompressed(nil, compress: true, compress_threshold: 1)
   end
 
   def test_small_string_with_default_compression_settings
@@ -178,7 +178,7 @@ module CacheStoreBehavior
   end
 
   def test_small_string_with_low_compress_threshold
-    assert_compressed(SMALL_STRING, compress: true, compress_threshold: 2)
+    assert_compressed(SMALL_STRING, compress: true, compress_threshold: 1)
   end
 
   def test_small_object_with_default_compression_settings
@@ -229,6 +229,25 @@ module CacheStoreBehavior
     assert_uncompressed(LARGE_OBJECT, compress: true, compress_threshold: 1.megabyte)
   end
 
+  def test_incompressable_data
+    assert_uncompressed(nil, compress: true, compress_threshold: 1)
+    assert_uncompressed(true, compress: true, compress_threshold: 1)
+    assert_uncompressed(false, compress: true, compress_threshold: 1)
+    assert_uncompressed(0, compress: true, compress_threshold: 1)
+    assert_uncompressed(1.2345, compress: true, compress_threshold: 1)
+    assert_uncompressed("", compress: true, compress_threshold: 1)
+
+    incompressible = nil
+
+    # generate an incompressible string
+    loop do
+      incompressible = SecureRandom.bytes(1.kilobyte)
+      break if incompressible.bytesize < Zlib::Deflate.deflate(incompressible).bytesize
+    end
+
+    assert_uncompressed(incompressible, compress: true, compress_threshold: 1)
+  end
+
   def test_cache_key
     obj = Object.new
     def obj.cache_key