Skip to content
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

ActiveSupport::Cache::Store(s) fails to autoload classes #8167

Closed
urielka opened this issue Nov 10, 2012 · 15 comments
Closed

ActiveSupport::Cache::Store(s) fails to autoload classes #8167

urielka opened this issue Nov 10, 2012 · 15 comments

Comments

@urielka
Copy link
Contributor

urielka commented Nov 10, 2012

Environment:

  1. Ruby 1.9.3-p327 (happens also in p0)
  2. Rails 3.2.8 (happens also in 3.2.3)

Steps to reproduce:

  1. create a new project using: rails new autoload_bug
  2. create some model: rails g model user
  3. rails c and then type this code:
Rails.cache.write("test",User)
  1. close that irb, rails c and type:
Rails.cache.read("test")

Expected:

This should work like any other code that depends on Rails autoloading.

Actual:

It fails with the following error:

ArgumentError: undefined class/module User
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/activesupport-3.2.8/lib/active_support/cache.rb:587:in `load'
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/activesupport-3.2.8/lib/active_support/cache.rb:587:in `value'
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/activesupport-3.2.8/lib/active_support/cache.rb:324:in `block in read'
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/activesupport-3.2.8/lib/active_support/cache.rb:520:in `instrument'
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/activesupport-3.2.8/lib/active_support/cache.rb:315:in `read'
    from (irb):1
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/railties-3.2.8/lib/rails/commands/console.rb:47:in `start'
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/railties-3.2.8/lib/rails/commands/console.rb:8:in `start'
    from /home/uriel/.rvm/gems/ruby-1.9.3-p0/gems/railties-3.2.8/lib/rails/commands.rb:41:in `<top (required)>'
    from script/rails:6:in `require'
    from script/rails:6:in `<main>'

Why this is important?

This is important in development (in production config.cache_classes = true takes care of this) because if I just start my rails server and the first requests fetches some instance that the class wasn't already loaded (typical case the User object) it will crash.

I have seen other people ran into this problem and their conclusion was either enable config.cache_classes (yuck) or loading all the models at app start.

Note:
I used the class and not an instance as creating an instance requires creating a database table and I wanted to keep it simple.
It happens with instances too.

The source of this bug:

All stores (from what I checked) works with Marshal#load and it seems the autoloading mechanism in rails doesn't work for Marshal#load (I guess because it is native code)

Related links:

Other people that had this bug:
http://www.philsergi.com/2007/06/rails-memcached-undefinded-classmodule.html
http://stackoverflow.com/questions/4202352/memcached-problem-with-rails-3-object-isnt-being-deserialized-the-second-time
http://blog.endpoint.com/2012/08/rails-3-activerecord-caching-bug-ahoy.html

Possible solution:
http://stackoverflow.com/questions/3531588/memcached-as-an-object-store-in-rails

@fxn
Copy link
Member

fxn commented Nov 10, 2012

Hi @urielka.

First of all, this bug report is really good, a model! Thanks for taking the time of writing it.

This is a known gotcha. The underlying problem is that, as you mentioned, Marshal.load tries to deserialize the packed data, and it does that in a way that does not trigger the const_missing hook set by Active Support for constant autoloading.

The traditional fix for that is to make sure cached classes are loaded before the cache is used. This is not what we'd call elegance though :).

I think rescue had some performance penalty in the past, but I have just benchmarked loading with and without a rescue clause, and I see no difference in 1.9.3.

I believe Active Support could decorate Marshal.load directly, which is the core of this problem and used by other cache stores, adding a workaround to it similar to the ones listed in the links you pass.

I'd like to know the opinion of other core team members though, /cc @jeremy @NZKoz.

@urielka
Copy link
Contributor Author

urielka commented Nov 10, 2012

Thanks! happy to help.

The thing about this known gotcha is that is not documented anywhere(except blog posts) and breaks another feature of rails(autoloading).
I consider this(first hit on google for rails cache) to be the documentation: http://guides.rubyonrails.org/caching_with_rails.html

Even if the rescue clause causes performance problems it could be enabled if config.cache_classes is off or if Rails.env.development?
The minimum would be to warn you that using the cache without cache_classes can results in this problem.

I don't mind even writing a patch,should I do it on top of 3.2.x or 4?

@NZKoz
Copy link
Member

NZKoz commented Nov 10, 2012

Historically we rescued LoadError, used regexps to pull out the missing constant from the error message, then re-tried the unmarshal. Pretty crazy stuff.

I don't think 'performance problems' are a reason to remove that code, but 'that's crazy' is :).

Perhaps as @urielka recommends you could just enable it for development mode or when cache_classes is off?

@fxn
Copy link
Member

fxn commented Nov 10, 2012

Problem with that approach is that we do not eager load everything that's in autoload_paths.

For me, simply adding the rescue to try constantize is fine.

@NZKoz
Copy link
Member

NZKoz commented Nov 10, 2012

The better long term fix would be for Marshal to call those hooks, but that's highly unlikely to change at this point.

I've definitely no objection to the code being added, however it's trivial to work around in production mode, generally people only hit this in development when they're caching reloaded classes.

@urielka
Copy link
Contributor Author

urielka commented Nov 11, 2012

The problem is indeed in development but I don't think that development should be treated as 2nd class citizen :)
I guess that Marshal doesn't call the const_missing hooks for performance reason (or nobody needed it).

I don't think that in production this code should run at all,in production LoadError actually means that something is missing (assuming cache_classes is true).

If there is no objection to the idea of adding this code,I can write a fix for this but I need to know on top of which branch to write it.

@NZKoz
Copy link
Member

NZKoz commented Nov 11, 2012

Start on master, we can backport if the fix looks straightforward

@fxn
Copy link
Member

fxn commented Nov 11, 2012

@urielka no, no, as I said before not all files in autoload_paths are eager loaded. If you deserialize in production it could still be the case that the class is not loaded.

Do it simple, no conditionals.

@urielka
Copy link
Contributor Author

urielka commented Nov 12, 2012

Will do :)

@urielka
Copy link
Contributor Author

urielka commented Nov 12, 2012

If I use Kernel#autoload then Marshal.load works just fine and there is no need for this hack.
But I see there is another(?) autoloading mechanism in Rails (active_support/dependencies.rb) that is used and it has this problem(which I understand is used for models).

@fxn : Why can't we just use Kernel#autoload ( or ActiveSupport::Autoload) for this?

@fxn
Copy link
Member

fxn commented Nov 12, 2012

The constant autoloading mechanism cannot be implemented with Kernel#autoload.

There are a few reasons, some may have dirty workarounds (like the fact that autoload uses require, a custom one indeed), but the main problem is namespaces. The autoload macro can only be passed a constant name, not a constant path, and has to be invoked within/on the parent namespace.

If namespaces are implicit (there is only a directory in the file system) we could perhaps still do something because AS creates a module on the fly that we control, but when they do have a file that defines them we'd need to execute that file. And that opens a can of worms, because files are supposed to be loaded only on demand and that file in particular could use constants in the top-level. They could indeed trigger loading of child constants when your autoloads are not yet in place.

@urielka
Copy link
Contributor Author

urielka commented Nov 13, 2012

Cool so I am testing this issue against ActiveSupport::Dependecies in my tests.

I will post more as I progress.

@urielka
Copy link
Contributor Author

urielka commented Nov 17, 2012

Pull request(no idea why it shows only commits): #8246

@fxn @NZKoz what you think about my solution? sorry for the ugly tests but it is quite hard to "unload" classes in ruby

urielka added a commit to urielka/rails that referenced this issue Dec 1, 2012
@fxn fxn closed this as completed in 60edece Dec 1, 2012
@cfabianski
Copy link
Contributor

Is there any plan to back-port this on Rails 3.2 ?

dementrock pushed a commit to dementrock/rails that referenced this issue Oct 19, 2013
…t/dependecies.rb) (issue rails#8167)

Conflicts:
	activesupport/CHANGELOG.md
	activesupport/lib/active_support/cache/mem_cache_store.rb
	activesupport/test/caching_test.rb
	guides/source/active_support_core_extensions.md
@Jesus
Copy link

Jesus commented Sep 5, 2016

I'm having this problem in my Rails 3.2 application.

Rather than monkey patching Rails in my application to solve it, I would like to send a PR and have it fixed in the next minor.

Would you be interested in merging that PR?

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

No branches or pull requests

5 participants