New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ActiveSupport::Cache compression #32539

Merged
merged 2 commits into from Apr 12, 2018

Conversation

@chancancode
Member

chancancode commented Apr 12, 2018

On Rails 5.2, when compression is enabled (which it is by default), the actual value being written to the underlying storage is actually bigger than the uncompressed raw value.

This is because the @marshaled_value instance variable (typically) gets serialized with the entry object, which is then written to the underlying storage, essentially double-storing every value (once uncompressed, once possibly compressed).

This regression was introduced in #32254.

Fix incoming.

Add failing test for compression bug
On Rails 5.2, when compression is enabled (which it is by default),
the actual value being written to the underlying storage is actually
_bigger_ than the uncompressed raw value.

This is because the `@marshaled_value` instance variable (typically)
gets serialized with the entry object, which is then written to the
underlying storage, essentially double-storing every value (once
uncompressed, once possibly compressed).

This regression was introduced in #32254.
Fix `ActiveSupport::Cache` compression
(See previous commit for a description of the issue)

@chancancode chancancode changed the title from [WIP] Fix ActiveSupport::Cache compression to Fix ActiveSupport::Cache compression Apr 12, 2018

@chancancode

This comment has been minimized.

Member

chancancode commented Apr 12, 2018

Ready for review!

@dhh dhh assigned jeremy and sgrif Apr 12, 2018

@rafaelfranca rafaelfranca merged commit b7760eb into rails:master Apr 12, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Apr 12, 2018

Merge pull request #32539 from chancancode/anticompress
Fix ActiveSupport::Cache compression
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 12, 2018

Thank you! As always, outstanding PR and investigation.

@chancancode chancancode deleted the chancancode:anticompress branch Apr 12, 2018

def compress!
@value = Zlib::Deflate.deflate(marshaled_value)
@compressed = true
defined?(@compressed)

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Apr 18, 2018

Contributor

was it changed intentionally since defined?(@compressed) ? @compressed : false looks correct?

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 18, 2018

Member

Yes. Ruby semantics of query methods don't require us to return false or true so nil is totally fine and inline with our guidelines.

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Apr 18, 2018

Contributor

@rafaelfranca
Ruby semantics is fine. I have another concern here since I see that behavior of this method is changed here.
defined? returns a string describing its argument by http://ruby-doc.org/core-2.5.0/doc/keywords_rdoc.html
Example:

@compressed = nil # or @compressed = false
defined? @compressed # => "instance-variable"

That means if we used @compressed like nil or false, we would need to change this method to the previous version in order to get right behavior.
I would recomend to initialize @compressed in the controller in order to get rid of using defined? there.
if it is a good reason, I'll send a patch?

diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb
index d769e2c..317723b 100644
--- a/activesupport/lib/active_support/cache.rb
+++ b/activesupport/lib/active_support/cache.rb
@@ -719,6 +719,7 @@ def initialize(value, compress: true, compress_threshold: DEFAULT_COMPRESS_LIMIT
         @version    = version
         @created_at = Time.now.to_f
         @expires_in = expires_in && expires_in.to_f
+        @compressed = nil

         compress!(compress_threshold) if compress
       end
@@ -798,7 +799,7 @@ def compress!(compress_threshold)
         end

         def compressed?
-          defined?(@compressed)
+          @compressed
         end

         def uncompress(value)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 18, 2018

Member

The current implementation is fine. A string is a truthy value and that is only want we need. @compressed will never be @nil or @false because we serialize this instance using marshal and we don't want to always serialize @compressed and your implementation always serialize @compressed.

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Apr 18, 2018

Contributor

You are right. Sorry for distraction.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 18, 2018

Member

No problem! thank you for double checking the implementation.

@zquestz

This comment has been minimized.

zquestz commented Jul 25, 2018

PLEASE cut a 5.2.1 release with this fix. It is blocking our upgrade path to Rails 5.2.x...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment