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

Cache entry conversion problem from 3.2 to 4.1 #17923

Closed
jeremywadsack opened this Issue Dec 4, 2014 · 16 comments

Comments

Projects
None yet
5 participants
@jeremywadsack
Copy link
Contributor

jeremywadsack commented Dec 4, 2014

I'm having a problem with cache data upgrading an app from 3.2 to 4.1.

Under 3.2 a cache Entry's value might be a hash as follows:

$ rails console
Loading development environment (Rails 3.2.21)
2.1.5 :001 > Rails.cache.write('my_key', {a:1})
 => true
2.1.5 :002 > Rails.cache.read('my_key')
 => {:a=>1}

Under 4.1 this is being returns as a string:

$ rails console
Loading development environment (Rails 4.1.8)
2.1.5 :001 > Rails.cache.read('my_key')
 => "\x04\b{\x06:\x06ai\x06"

This seems to me to be related to the fact that under Rails 4 we no longer Marshal.load the data that was Marshal.dumped into the cache Entry in Rails 3. See this commit lines L585 vs R562.

I see the discussion in #9494 about conversion issue, although I'm not sure I follow all of it. I notice that at several points there's discussion about having to rebuild the cache but there's no discussion of a breaking change to the cache in the upgrade Guide. Is cached data not expected to be portable?

I have created an example repository that you can clone and reproduce the issue using the built-in file-system store. Instructions to reproduce this are in the README.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Dec 4, 2014

Is cached data not expected to be portable?

In general, I would say no.

@jeremywadsack

This comment has been minimized.

Copy link
Contributor Author

jeremywadsack commented Dec 4, 2014

Is cached data not expected to be portable?

In general, I would say no.

Wow. That's unfortunate for us.

The discussion in #9494 led me to believe the goal was to port the cache data:

fxn: What about transition code for Rails 3?

and (implying that this is not ideal)

jeremy: The safe path is to change the cache namespace and roll out with a cold cache. That's daunting: users hit a sluggish app, and it's very hard to gauge before/after app performance to check for regressions.

And from the thread that introduced these changes:

Xavier Noria: Do you think we could hack something in order to support already cached values on deserialization to ease upgrading? Otherwise all caches would need to be wiped.

and

Brian Durand: It should now be backward compatible with older cache entries as well.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Dec 6, 2014

Is there a specific actionable issue being raised here? We cannot change the format used in 4.1 at this point.

@jeremywadsack

This comment has been minimized.

Copy link
Contributor Author

jeremywadsack commented Dec 6, 2014

The "actionable issue" would be to document either a breaking change in the cache storage or the fact that cached items are "not expected to be portable" between releases. I'll open a pull request to update the upgrade docs for the former.

I wasn't sure if your previous response was an affirmation that the data storage changed or an off-hand comment. Frankly I was hoping that someone would point me to an error in my interpretation and show that the data was portable between releases.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Dec 6, 2014

/cc @rafaelfranca

On Fri, Dec 5, 2014, 9:20 PM Jeremy Wadsack notifications@github.com
wrote:

The "actionable issue" would be to document either a breaking change in
the cache storage or the fact that cached items are "not expected to be
portable" between releases. I'll open a pull request to update the upgrade
docs for the former.

I wasn't sure if your previous response was an affirmation that the data
storage changed or an off-hand comment. Frankly I was hoping that someone
would point me to an error in my interpretation and show that the data was
portable between releases.


Reply to this email directly or view it on GitHub
#17923 (comment).

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 8, 2014

I believe there is a bug there. Even that I agree we should not expect the cache to be portable we made the entry in a way that it could be ported between versions.

This patch should fix:

diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb
index fc470f3..4cbea61 100644
--- a/activesupport/lib/active_support/cache.rb
+++ b/activesupport/lib/active_support/cache.rb
@@ -587,7 +587,7 @@ module ActiveSupport

       def value
         convert_version_4beta1_entry! if defined?(@v)
-        compressed? ? uncompress(@value) : @value
+        compressed? ? uncompress(@value) : Marshal.load(@value)
       end

       # Check if the entry is expired. The +expires_in+ parameter can override

But I'm not sure if it is safe to apply at this point.

@jeremy @matthewd WDYT?

@rafaelfranca rafaelfranca added this to the 4.0.13 milestone Dec 8, 2014

@rafaelfranca rafaelfranca self-assigned this Dec 8, 2014

@jeremywadsack

This comment has been minimized.

Copy link
Contributor Author

jeremywadsack commented Dec 15, 2014

@rafaelfranca I think that patch would break existing 4.0 / 4.1 caches that are not Marshaled. So maybe something more like this:

--- a/activesupport/lib/active_support/cache.rb
+++ b/activesupport/lib/active_support/cache.rb
@@ -587,7 +587,7 @@ module ActiveSupport

       def value
         convert_version_4beta1_entry! if defined?(@v)
-        compressed? ? uncompress(@value) : @value
+        compressed? ? uncompress(@value) : Marshal.load(@value) rescue @value
       end

       # Check if the entry is expired. The +expires_in+ parameter can override

Also, I thought the point was to get rid of the Marshal.load where possible to make this faster.

I'm working on a cache proxy gem that we'll use to migrate (if needed) to the new cache format. But I'm not sure when I'll have time to wrap that up. But with the above patch, I guess that wouldn't be necessary.

rafaelfranca added a commit that referenced this issue Dec 29, 2014

Expire Marshaled Rails 3 cache entries
To give a better backward compatibility path we will expire all cache
entries that are marshaled without compressing instead of returning the
raw value.

Fixes #17923
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jan 2, 2015

I guess at this point we can't do anything to ensure Rails 3 cache entries will be fully backwards compatible at Rails 4.

My attempt at #18247 will break rack-cache entries so I don't think we have any solution that will satisfy all requirements at this point.

That said I'm closing this issue. Thank you for reporting.

@jeremywadsack

This comment has been minimized.

Copy link
Contributor Author

jeremywadsack commented Jan 2, 2015

I'll admit that I had hoped that 18 months after release of Rails 4 I wouldn't run into issues like this, so I can only assume that other Rails users are not using Rails cache like we are and are perfectly comfortable dumping their cache on upgrade (and clearly have).

Our cache takes about two weeks to warm from scratch so we can't run from an empty cache. I think the best bet is to warm a Rails 4 environment side-by-side while the Rails 3 one is still in production and then flip the switch.

Perhaps we should be using Redis directly and skipping the Rails.cache.fetch method, but it's got so much nice stuff going on. :)

Thanks for giving it a shot.

@mattolson

This comment has been minimized.

Copy link

mattolson commented Jul 26, 2016

I was caught by surprise by this. I read many guides on the migration from 3.2 to 4.2 (including the official one) and did not find any mention of this. Upon deployment of the upgrade to our high traffic service, we had very high error rates due this incompatibility, ultimately cascading into downtime. This should be highlighted in the official migration guide. It appears that the changes in #17943 have since been removed.

@jeremywadsack Did you ever finish the gem you mention to help with the migration? Or do you have a gist to share? I will need to decide whether to build something or purge the cache during the upgrade.

@jeremywadsack

This comment has been minimized.

Copy link
Contributor Author

jeremywadsack commented Jul 26, 2016

@mattolson: @rafaelfranca reverted the documentation change in 6961afe, but that was before we discovered that we couldn't fix the bug. I agree that the documentation change for #17943 should probably exist. I think that with Rails 5 shipped it's not going to be updated at this point.

The gem I was going to build I removed because, as mentioned in this thread, there is a security issue if you are marshalling data that may or may not have been marshalled to begin with. Instead we spun up a new cache for the Rails 4 data and migrated using https://github.com/keylimetoolbox/rails_cache_migrator. That worked well for us on two separate migrations of tens of thousands of keys each.

@mattolson

This comment has been minimized.

Copy link

mattolson commented Jul 26, 2016

@jeremywadsack Thank you!

@mhuggins

This comment has been minimized.

Copy link

mhuggins commented Dec 28, 2016

I wish this change had been documented. This bit us as well.

@jeremywadsack

This comment has been minimized.

Copy link
Contributor Author

jeremywadsack commented Dec 28, 2016

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 29, 2016

As that change was made in the upgrading guide it is released using the latest stable release as base so it is already documented in the guides. Backporting to 4.0.0 would only be present in the website if a new release of 4.0 was made (it is not supported anymore) and if people put the v4.0 in the end of the URL of the guide, so I don't think it is worth to backport.

@mhuggins as you can see it was documented.

@rafaelfranca rafaelfranca reopened this Dec 29, 2016

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 29, 2016

Sorry, We removed the documentation to try to fix. But I just added it back Thanks

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