Skip to content

Cache entry conversion problem from 3.2 to 4.1 #17923

New issue
New issue
Closed
Closed
Cache entry conversion problem from 3.2 to 4.1#17923
Assignees
rafaelfranca
Labels
activesupport
Milestone
 
4.0.13
@jeremywadsack

Description

@jeremywadsack
jeremywadsack
opened on Dec 4, 2014
Contributor

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.

Activity

sgrif

sgrif commented on Dec 4, 2014

@sgrif
sgrif
on Dec 4, 2014
Contributor

Is cached data not expected to be portable?

In general, I would say no.

jeremywadsack

jeremywadsack commented on Dec 4, 2014

@jeremywadsack
jeremywadsack
on Dec 4, 2014
ContributorAuthor

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

sgrif commented on Dec 6, 2014

@sgrif
sgrif
on Dec 6, 2014
Contributor

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

jeremywadsack

jeremywadsack commented on Dec 6, 2014

@jeremywadsack
jeremywadsack
on Dec 6, 2014
ContributorAuthor

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.

jeremywadsack
mentioned this on Dec 6, 2014
sgrif

sgrif commented on Dec 6, 2014

@sgrif
sgrif
on Dec 6, 2014
Contributor

/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

rafaelfranca commented on Dec 8, 2014

@rafaelfranca
rafaelfranca
on Dec 8, 2014
Member

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
added
activesupport
on Dec 8, 2014
rafaelfranca
added this to the 4.0.13 milestone on Dec 8, 2014
rafaelfranca
self-assigned this
on Dec 8, 2014
jeremywadsack

jeremywadsack commented on Dec 15, 2014

@jeremywadsack
jeremywadsack
on Dec 15, 2014
ContributorAuthor

@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 references this issue on Dec 29, 2014

Expire Marshaled Rails 3 cache entries

a26048c
rafaelfranca
mentioned this on Dec 29, 2014
rafaelfranca

rafaelfranca commented on Jan 2, 2015

@rafaelfranca
rafaelfranca
on Jan 2, 2015
Member

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.

rafaelfranca
closed this as completedon Jan 2, 2015
jeremywadsack

jeremywadsack commented on Jan 2, 2015

@jeremywadsack
jeremywadsack
on Jan 2, 2015
ContributorAuthor

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

mattolson commented on Jul 26, 2016

@mattolson
mattolson
on 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

jeremywadsack commented on Jul 26, 2016

@jeremywadsack
jeremywadsack
on Jul 26, 2016 · edited by jeremywadsack
ContributorAuthor

@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

mattolson commented on Jul 26, 2016

@mattolson
mattolson
on Jul 26, 2016

@jeremywadsack Thank you!

mhuggins

mhuggins commented on Dec 28, 2016

@mhuggins
mhuggins
on Dec 28, 2016

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

jeremywadsack

jeremywadsack commented on Dec 28, 2016

@jeremywadsack
jeremywadsack
on Dec 28, 2016 via email
ContributorAuthor
rafaelfranca

rafaelfranca commented on Dec 29, 2016

@rafaelfranca
rafaelfranca
on Dec 29, 2016
Member

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
reopened this on Dec 29, 2016
rafaelfranca

rafaelfranca commented on Dec 29, 2016

@rafaelfranca
rafaelfranca
on Dec 29, 2016
Member

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

rafaelfranca
closed this as completedon Dec 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

activesupport

Type

No type

Projects

No projects

Milestone

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @rafaelfranca@mattolson@mhuggins@jeremywadsack@sgrif

      Issue actions
        Cache entry conversion problem from 3.2 to 4.1 · Issue #17923 · rails/rails