ActionDispatch::RailsMetaStore returns corrupt Rack responses with MemoryStore

[nbibler]

This bug causes cached asset responses to be invalid after two successful requests. This means that with ActionController caching enabled in production, assets will not be returned correctly to clients. To us, this seems to be an RC-stopping issue.

In the Rails 3.1.0.beta1, using:

• rails (3.1.0.beta1)
• rack (1.3.0.beta)
• rack-cache (1.0.1)

There is a reproducible bug when ActionController caching is enabled (in production), where ActionDispatch::RailsMetaStore, along with ActiveSupport::Cache::MemoryStore, will ultimately cause Rack::Cache to return an HTTP 0 (invalid!) response to Rack.

The implementation of Rack::Cache works by storing hashes into the cache to retrieve on future requests. Those hashes are written and pulled as necessary for invalidation.

The basic lifecycle looks like this:

1. First request, cache empty, find file, generate fingerprint, append to file name, store request response hash information to cache, serve content back to client with a miss, store header response on the cache,
2. Second request, identify matching file request based on name and fingerprint, read from cache, read and drop the X-Status header from the cached hash and return that hash forward to Rack.
3. Third request, identify matching file request based on name and fingerprint, read from cache, read and drop the X-Status header (except it's already been DELETED!), find no status, return HTTP 0 to rack.
4. All subsequent matches follow Status: 500 Internal Server Error private method `split' called for Mime::Type in  #3.

So, it seems to be that because the MemoryStore cache is handing back the hash object directly, Rack::Cache is then manipulating it before handing it upstream to Rack. In most cases, that's fine, except that because MemoryStore is handing back the actual object, and Rack::Cache is manipulating it, the cache object is ultimately, unknowingly being manipulated. So, subsequent finds are returning the "bad" object.

[nbibler]

So a quick fix for this in one of our applications is to run a modified version of Rack::Cache which removes the deletion call on X-Status. I'm not sure whether or not this will negatively affect any other functionality of Rack::Cache or how it might affect other Rails cache stores. But, for this particular application, it is currently running fine with that fix in production.

[josevalim]

This just happens with memory store? Shouldn't memory store return a duplicated object instead of the same object to be more compatible with the other stores? /cc @jeremy

[nbibler]

Yes, this appears to only be an issue with the memory store. We did manually test with the file store and did not have a failure.

We have not yet tested with the other stores (memcached, etc.). I know @rubymaverick attempted to add a dup to theread method, but in his limited attempt, it didn't seem to immediately fix the issue.

[ericallam]

Yes, it only happens with the memory store (I worked with @nbibler on discovering this issue). We also tested with FileStore and it worked fine.

[josevalim]

Just duping probably won't work because if you have an array inside a hash it won't be duped :(. Maybe we can Marshal.dump and Marshal.load?

[tenderlove]

@josevalim +1 on marshal dump / load. It's a good trick to get a deep clone.

[nbibler]

@tenderlove Right.. I'm actually surprised ActiveSupport doesn't provide a deep_dup or deep_clone helper to wrap load/dump of an object. I know that there are a few Ruby objects out there (mostly legacy C-based) that don't support marshal dump - Ruby's OpenSSL classes come to mind ... but the need for it comes up quite often.

As far as dump/loading with every read from the memory cache, it almost seems a bit excessive for something that is supposed to be a low latency, short-circuit for caching long running methods. But, I agree that it may be the quickest option to solve this issue and the implementation can be refactored at a later date, if necessary.

[josevalim]

Another good reason to use marshaling is that it will be closer to the other cache stores, so swapping one store from the other won't fail because of dump/load. I think this is how it was used to work but it was changed somewhere down the road. We need to confirm with @jeremy (<3 <3 <3 <3 <3 kemper call!).

[rtomayko]

I'm going to make the Marshal.dump / load change upstream and get a release out. The memory store should definitely blow up in the same way as the other stores. In fact, I think the memory store should Marshal.dump before writing to its backing metastore hash and Marshal.load on the way out for consistency with the other stores.

[rtomayko]

rack-cache 1.0.2 is on rubygems.org with the marshal dump / load fix:

rtomayko/rack-cache@28bbc62

Can someone give that a shot to verify it addresses the issue?

[josevalim]

Thanks @rtomayko ! And yeah, we should totally dump on write and load on read.

[ericallam]

@rtomayko, that looks good but it doesn't fix the issue with Rails. I think we just need to do the Marshal.load/dump in the RailsMetaStore read/write methods.

[rtomayko]

Oh, I see. I thought it was the Heap store in Rack::Cache that was causing problems. Thanks.

[jeremy]

Seems crazy to marshal into a memory store, though.

How about not modifying the cached value in-place?