mhgbrown / cached_resource

Caching for ActiveResource
MIT License
80 stars 28 forks source link

Cache store freezing object (Rails 3.1) #4

Closed ianejames closed 12 years ago

ianejames commented 12 years ago

In Rails 3.1.3 the MemoryCache (and perhaps others) inexplicably freezes the stored object on read:

cache = ActiveSupport::Cache::MemoryStore.new
s = ""
s.frozen? # false
cache.write(:s, s)
s.frozen? # false
cache.read(:s)
s.frozen? # true

This manifests itself when an a resource is retrieved twice and the first one is altered:

a1 = Account.find(1) # From remote service
a1.frozen? # false
a2 = Account.find(1) # From cache
a1.frozen? # true
a1.name = "Checking" # Raises TypeError: can't modify frozen object

A simple workaround would be to dup the object before storing. Updated line 95 of cached_resource/caching:

result = cached_resource.cache.write(key, object.dup, :expires_in => cached_resource.generate_ttl)

This issue appears to have been fixed in Rails 3.2, so this change is only necessary for backwards compatibility.

mhgbrown commented 12 years ago

Huh, that is interesting! I have no foundation for this concern, but I am wondering if there might be some unexpected results with dup-ing, especially with ActiveRecord. Again, I don't know of any concrete thing, but just something I wanted to bring up.

I'd also like to come up with a way to test this consistently.

ianejames commented 12 years ago

Was that supposed to be "ActiveResource"?

The object is already duped (dupped, dup-ed?) on read, so I can't imagine a second dup doing any (more) damage.

An alternate solution would be to always return the object from cache. So second, third, etc retrievals would be unchanged, but the first retrieval would be altered to store the object using the cache-write code, then call the cache-read code and return that result instead of the originally retrieved object. Since the cache-read code already dups the object we don't need to worry about freezing. And that means each retrieval would work exactly identically because it calls the same code. This option would require a little more work, but the more I think about it the better it gets. Thoughts?

I'll look through the tests and see if I can come up with anything.

mhgbrown commented 12 years ago

Oops, yeah that's what I meant :)

I dig your alternative solution. I think the extra work is totally worth it.

On the testing front, I was experimenting with swapping out ActiveSupports, but dropped it to work on some other stuff.