Skip to content

Fix #8167 - adding autoloading support for caching #8246

Merged
merged 1 commit into from Dec 1, 2012

9 participants

@urielka
urielka commented Nov 17, 2012

I wrote a (quite) simple fix for supporting autoloading in ActiveSupport::Cache (which relays on Marshal#load/dump).

Changes:

  • Patched Marshal#load so it works with constant autoloading in active_support/core_ext/marshal.rb
  • Made sure this patch is used in MemCacheStore and FileStore (NullStore and MemStore don't marshal objects)
  • Extracted few useful methods in dependecies_test.rb into dependecies_test_helpers.rb and used them in object_serializer/dependecies_test/caching_test.rb
  • Modified caching_test.rb with two cases that trigger the autoloading.

Testing done:
Run the tests in ActiveSupport including my new tests

Environment:
ruby 1.9.3p0 using rails-dev-box (great project!)

@fxn
Ruby on Rails member
fxn commented Nov 17, 2012

Cool, thanks for this work. Let me give you some feedback on the proposal:

While I understand the intention of introducing the ObjectSerializer, as I suggested previously I'd prefer to decorate Marshal.load. I mean, alias_method_chain decoration.

The ObjectSerializer class would introduce a new contract that all cache stores, core and plugins, should use. Note that the very memcache store the patch updates does not use the new dump. So it is no following the contract, if we change dump later the store won't use the new version.

The thing is that the mismatch is not really in ActiveSupport::Cache vs constant autoloading. That's a symptom of the core issue. The core of this problem is that constant autoloading does not play well with Marshal.load. That affects to any usage of Marshal.load, either there or anywhere else, including direct usage by the end user. And I believe that is what AS has to address.

Regarding the tests, while we are preventing an exception from being raised, what we really want to test is that loading the serialized data yields what we dumped. So instead of basing the tests on assert_nothing_raised it would be better to write assertions about the objects that we expect to load.

Then a couple of stylistic points:

  • Would be nice that the code follows our code conventions. In particular a space after the hash that opens a comment, and a space after commas in arguments/parameters.

  • The regexp that captures the class name from the exception message could use an alternative delimiter to avoid the need of a backslash, and the anchor $ is redundant, no need to put that anchor.

Finally, this needs to be registered in the CHANGELOG of Active Support.

Could you please give it a second pass?

@guilleiguaran
Ruby on Rails member

Agree with @fxn, I would prefer to have this in Marshal.load instead of AS cache stores

@urielka
urielka commented Nov 19, 2012

I will give it another shot :)

I am mostly against monkey-patching core library(probably the Python guy in me) but in this case it does make sense.

@urielka
urielka commented Nov 19, 2012

BTW the fix for Marshal should be in active_support/core_ext/marshal.rb right?

When is it loaded? I see that core_ext.rb loads everything but who loads him?

@fxn
Ruby on Rails member
fxn commented Nov 19, 2012

Yep, that file is the one.

The file will be automatically loaded by active_support/core_ext.rb in Rails applications by default.

This new feature has to be also documented in the AS guide.

@urielka
urielka commented Nov 24, 2012

I hope I got it right this time :)

@fxn I tried to incorporate all your comments,what you think about this?

@senny
Ruby on Rails member
senny commented Nov 24, 2012

really like this PR. This makes a lot of hacks useless we put in place to work around this issue. :purple_heart:

@fxn
Ruby on Rails member
fxn commented Nov 24, 2012

@urielka thanks, I'll try to have a look tomorrow.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Nov 25, 2012
activesupport/CHANGELOG.md
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* Patched Marshal#load to work with constant autloading.
+ Fixes autoloading with cache stores that relay on Marshal(MemCacheStore and FileStore). [fixes #8167]
+ *Uriel Katz*
@carlosantoniodasilva
Ruby on Rails member

Can you fix the indent here please? Also a blank line to separate your name :)

@urielka
urielka added a note Nov 25, 2012

I tried to follow the conventions in the file,I didn't see a newline :)

About indentation,how should it be exactly? I indented all the lines to start at the same place

@carlosantoniodasilva
Ruby on Rails member

We kinda follow that one-line changelogs have the name at the end, multiline have the name in a new line separated by a blank line.

About the indentation, if you look the file through github you'll notice that it contains only two spaces, whereas the others have 4. Are you using tabs by any chance? If so, please change to spaces.

@urielka
urielka added a note Nov 26, 2012

I will fix it,I use 2 spaces as tab :)

@carlosantoniodasilva
Ruby on Rails member

Ok, great, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Nov 25, 2012
activesupport/lib/active_support/core_ext/marshal.rb
@@ -0,0 +1,20 @@
+module Marshal
+ class << self
+ def load_with_autoloading(source)
+ begin
+ load_without_autoloading(source)
+ rescue ArgumentError, NameError => exc
+ if exc.message.match(%r|undefined class/module (.+)|)
+ # try loading the class/module
+ $1.constantize
+ # if it is a IO we need to go back to read the object
+ source.rewind if source.respond_to?(:rewind)
+ retry
+ end
+ raise exc
@carlosantoniodasilva
Ruby on Rails member

I think that adding an else condition here could make it clearer, wdyt?

@urielka
urielka added a note Nov 26, 2012

if am quite used to short-circuiting (like if-return without else) but it does seem more readable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Nov 25, 2012
activesupport/test/caching_test.rb
@@ -1,6 +1,8 @@
require 'logger'
require 'abstract_unit'
require 'active_support/cache'
+require 'dependecies_test_helpers'
+
@carlosantoniodasilva
Ruby on Rails member

:scissors: extra blank line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Nov 25, 2012
activesupport/test/caching_test.rb
+module AutoloadingCacheBehavior
+ include DependeciesTestHelpers
+ def test_simple_autoloading
+ with_autoloading_fixtures do
+ @cache.write('foo',E.new)
+ end
+
+ remove_constants(:E)
+ ActiveSupport::Dependencies.clear
+
+ with_autoloading_fixtures do
+ assert_nothing_raised do
+ loaded = @cache.read('foo')
+ assert_kind_of E, loaded
+ end
+
@carlosantoniodasilva
Ruby on Rails member

:scissors: extra blank line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Nov 25, 2012
activesupport/test/caching_test.rb
@@ -562,6 +564,51 @@ def test_middleware
end
end
+module AutoloadingCacheBehavior
+ include DependeciesTestHelpers
+ def test_simple_autoloading
+ with_autoloading_fixtures do
+ @cache.write('foo',E.new)
+ end
+
+ remove_constants(:E)
+ ActiveSupport::Dependencies.clear
+
+ with_autoloading_fixtures do
+ assert_nothing_raised do
+ loaded = @cache.read('foo')
+ assert_kind_of E, loaded
+ end
@carlosantoniodasilva
Ruby on Rails member

I think that assert_nothing_raised is not necessary, there's already an assertion inside the block, and "nothing raised" actually doesn't do any assertion. Let it just :boom: in case of an error. Update: same for the other calls :)

@urielka
urielka added a note Nov 26, 2012

This is part of the tests that I was a bit uncomfortable about..

In my first version I was only check that when loading values from cache no exception about missing class was raised.

and then @fxn pointed (rightfully) that it will be better to check that what I am reading from the cache is what I wrote.

so instead of:

assert_nothing_raised { @cache.read('foo') }

I thought about doing:

loaded = nil
assert_nothing_raised { loaded = @cache.read('foo')}
assert_kind_of E, loaded

And the it incarnated to this,but I guess that just check for values is better and letting exception to be thrown is more elegant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Nov 25, 2012
activesupport/test/core_ext/marshal_test.rb
@@ -0,0 +1,141 @@
+require 'abstract_unit'
+require 'active_support/core_ext/marshal'
+require 'dependecies_test_helpers'
+
+class MarshalTest < ActiveSupport::TestCase
+ include ActiveSupport::Testing::Isolation
+
+ def teardown
+ ActiveSupport::Dependencies.clear
+ remove_constants(:E,:ClassFolder)
+ end
+
+ def initialize(*args)
+ super
+ end
@carlosantoniodasilva
Ruby on Rails member

Necessary?

@urielka
urielka added a note Nov 26, 2012

Nope :) I forgot to remove it when I refactored this part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Nov 25, 2012
activesupport/test/core_ext/marshal_test.rb
+require 'dependecies_test_helpers'
+
+class MarshalTest < ActiveSupport::TestCase
+ include ActiveSupport::Testing::Isolation
+
+ def teardown
+ ActiveSupport::Dependencies.clear
+ remove_constants(:E,:ClassFolder)
+ end
+
+ def initialize(*args)
+ super
+ end
+
+ test "that Marshal#load still works" do
+ sanity_data = ["test", [1,2,3], {:a => [1,2,3]}, ActiveSupport::TestCase]
@carlosantoniodasilva
Ruby on Rails member

Please use the new hash style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Nov 25, 2012
activesupport/test/core_ext/marshal_test.rb
+
+ f.rewind
+ remove_constants(:E)
+ ActiveSupport::Dependencies.clear
+
+ with_autoloading_fixtures do
+ assert_nothing_raised do
+ loaded = Marshal.load(f)
+ assert_kind_of E, loaded
+ end
+ end
+ end
+ end
+
+private
+ include DependeciesTestHelpers
@carlosantoniodasilva
Ruby on Rails member

I don't think this actually makes the included methods private, does it? I think that's handled by whether they're private inside the module or not.

@urielka
urielka added a note Nov 26, 2012

Right I will just drop the private and move the include up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

Thanks @urielka :+1:

@fxn fxn commented on an outdated diff Nov 26, 2012
activesupport/CHANGELOG.md
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* Patched Marshal#load to work with constant autloading.
@fxn
Ruby on Rails member
fxn added a note Nov 26, 2012

Missing "o" in "autloading".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@urielka
urielka commented Nov 26, 2012

@fxn and @carlosantoniodasilva thanks for you comments and time I hope it is better now.
@senny thanks!

@carlosantoniodasilva
Ruby on Rails member

@urielka great, thanks!

@dubek
dubek commented Nov 29, 2012

:+1:

@fxn
Ruby on Rails member
fxn commented Nov 29, 2012

@urielka looking good, could you please rebase?

@urielka
urielka commented Nov 29, 2012

@fxn sure should I just rebase or rebase and squash it to another branch?
If I need to squash it,then I need to open a new pull request?

@steveklabnik
Ruby on Rails member
@urielka
urielka commented Nov 30, 2012

@fxn done :)

@fxn
Ruby on Rails member
fxn commented Nov 30, 2012

Excellent, I'll have a look today.

@fxn
Ruby on Rails member
fxn commented Dec 1, 2012

@urielka the patch still doesn't update the AS guide. Could you please add documentation there?

Since AS reopens core classes and modules it is our duty to document that very clearly so people are aware not only about the feature, but also about the method redefinition.

Also, if you don't mind it would be nice that the code followed the conventions in the existing code base, in particular a space after commas.

@urielka
urielka commented Dec 1, 2012

@fxn I updated the AS guide (I hope it is good enough)

About the coding conventions I tried to follow them and searched each comma to verify there is a space after it(found few that didn't have it and fixed them).

I followed everything written here:
http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#follow-the-coding-conventions

If there is still something that doesn't meet the coding conventions can you please pinpoint it? (I guess looking a the same code few times makes you blind to this kind of things).

Also I rebased from master.

@fxn
Ruby on Rails member
fxn commented Dec 1, 2012

Excellent, the AS guide docs need some copy-editing to follow the documentation conventions, but you've worked a lot on this proposal and I am happy to do them myself later.

If you rebase the PR so that it merges I'll apply.

@urielka
urielka commented Dec 1, 2012

@fxn done,thanks for all the help!

@fxn
Ruby on Rails member
fxn commented Dec 1, 2012

Thanks man!

@fxn fxn merged commit 60edece into rails:master Dec 1, 2012
@senny
Ruby on Rails member
senny commented Dec 1, 2012

Thanks!

@urielka
urielka commented Dec 1, 2012

Thank you all for the time and effort!

@senny senny referenced this pull request in QueueClassic/queue_classic Dec 2, 2012
Closed

proof of concept: Rails 4 Queue API #112

@tispratik

Is it possible to patch this on Rails 3?

@fxn
Ruby on Rails member
fxn commented Mar 10, 2014

@tispratik you mean by yourself? or whether Rails is going to patch 3-2-stable to add this?

@tispratik

Actually both. I want to know what is the minimum amount of code i can grab from this commit to monkeypatch my project and also if the rails team has a plan to patch Rails3.2 branch.

Thanks a lot!

@urielka
urielka commented Mar 10, 2014

@tispratik if you want to patch it by yourself then I think it should apply cleanly (minus tests I guess).
if not, just copy the core_ext/marshal.rb and require it as early as possible in your rails app

The minimum amount of code is just core_ext/marshal.rb.

@urielka
urielka commented Mar 10, 2014

Also take the code from here:
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/marshal.rb
The difference is:
1. removed begin which is not needed
2. added require needed for alias_method_chain

@urielka urielka deleted the urielka:uriel-fixed-8167 branch Mar 10, 2014
@fxn
Ruby on Rails member
fxn commented Mar 10, 2014

@tispratik this is not going to be backported, but following @urielka's instructions it would be easy to add this behaviour to your app.

@tispratik

Thank you very much for your prompt guidance @urielka and @fxn.
I really appreciate it.

@alediaferia

Regarding this patch, I'm having an issue caused by the retry in the rescue block which leads to an infinite recursion due to an "undefined class/module" exception being fired despite the constantize for ActiveRecord::ConnectionAdapters::Mysql2Adapter::Column.

I've reported it here: #19067

I cannot manage to reproduce on a brand-new app but can you give me hints on what to check? I already removed all non-rails gems from the call stack but I keep having this. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.