From 66df366268c4e5b17c9235b5a0c3aabcbd578e01 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Wed, 11 Apr 2018 22:54:20 -0700 Subject: Fix `ActiveSupport::Cache` compression (See previous commit for a description of the issue) --- activesupport/CHANGELOG.md | 7 +++ activesupport/lib/active_support/cache.rb | 67 +++++++++++----------- .../test/cache/behaviors/cache_store_behavior.rb | 23 +++++++- 3 files changed, 60 insertions(+), 37 deletions(-) (limited to 'activesupport') 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 # value.size 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 -- cgit v1.2.3