This project is archived and is in readonly mode.

#799 ✓resolved
Jim Lindley

MemoryStore cache contents should be immutable

Reported by Jim Lindley | August 11th, 2008 @ 05:25 PM | in 2.x

This was a side issue to ticket #785. Splitting out into it's own ticket for further discussion. MemoryStore cache contents can be altered by changing fetched objects outside of the cache.

"[T]he existing MemoryStore implementation is faulty because changes made to an object afterwards effect the cache. This happens in production as well. I don't know of any other caching technique that behaves this way, and I think it's one of the core problems with MemoryStore itself." (As explained by Ryan Bates in #785)

cache = ActiveSupport::Cache.lookup_store(:memory_store)
h = {:a => 1}
cache.write('foo', h)
h[:a] = 2'foo')[:a] # => 2

The proposal to fix this issue was to dup objects on write and outgoing as fetched, when possible. (Nil, False, etc, can't be dupped).

Comments and changes to this ticket

  • Jim Lindley

    Jim Lindley August 11th, 2008 @ 04:02 PM

    Aaargh, getting the code formatting wrong. Trying again, with Ryan's example.

    Existing behaviour:

        cache = ActiveSupport::Cache.lookup_store(:memory_store)
        h = {:a => 1}
        cache.write('foo', h)
        h[:a] = 2'foo')[:a] # => 2
  • Ryan Bates

    Ryan Bates August 11th, 2008 @ 04:23 PM

    • Tag changed from activesupport to activesupport, bug, patch

    As determined in the other ticket (#785), dup is an efficient way to resolve this issue. Here's an attachment which just adds this behavior.

    BTW, I think it's possible for you to update the original ticket and fix the formatting so the rest of the page isn't monospace. Thanks.

  • Jim Lindley

    Jim Lindley August 11th, 2008 @ 04:34 PM

    Doesn't look like I can edit the original. Wish there was a preview button. Stuck with monospaced.

  • Ryan Bates

    Ryan Bates August 11th, 2008 @ 04:51 PM

    There should be an "Edit Ticket" link in the blue bar at the top. At least that's available on my tickets.

  • Tom Lea

    Tom Lea August 11th, 2008 @ 04:57 PM

    Not sure if this is a show stopper but the following test (inserted in MemoryStoreTest) fails:

      class SimpleClass
        attr_accessor :value
        def initialize(value)
          @value = value
      def test_instance_variable_duping
        instance ="Pass"))
        @cache.write("instance", instance)
        instance.value.value = "FAIL!"
        assert_equal "Pass","instance").value.value

    No simple way around this, but it could be an argument in favour of serializing it despite the overhead.

  • Jim Lindley

    Jim Lindley August 11th, 2008 @ 05:04 PM

    Ryan, I get those edit links on my own Lighthouse projects, but not when I'm logged in here at the Rails project.

  • Jim Lindley

    Jim Lindley August 11th, 2008 @ 07:07 PM

    Fixed thanks to Rick and Will - the edit link shows up for me now.

  • josh

    josh August 11th, 2008 @ 09:01 PM

    • Title changed from “MemoryStore cache contents should be dup'd..” to “MemoryStore cache contents should be immutable”
    • State changed from “new” to “open”
    • Assigned user set to “josh”
    • Tag changed from activesupport, bug, patch to activesupport, patch

    I'm of the opinion that all cache store should be immutable. So freeze every object on write. If you need a mutable copy, dup at your own expense.

  • Ryan Bates

    Ryan Bates August 11th, 2008 @ 09:52 PM

    Requiring all objects sent to cache be immutable can really complicate the code. For example:

    all_cached ='Recipe.all')
    if all_cached.nil?
      all = Recipe.all
      Rails.cache.write('Recipe.all', all.dup.freeze)
      all = all_cached.dup
    # vs.
    all = Rails.cache.fetch('Recipe.all') { Recipe.all }

    To me this seems like something the framework should do, especially considering we have the convenient "fetch" method built in.

  • josh

    josh August 13th, 2008 @ 01:45 AM

    @Ryan I meant to have Store#write call freeze on the the given value. freeze should also be called on the retrieved value as well. Requiring "frozen?" to be true is silly as you just demonstrated :)

    You never want to modify objects in the cache store.

  • Ryan Bates

    Ryan Bates August 13th, 2008 @ 05:45 PM

    If the cache store calls freeze on write then the developer has to ensure every mutable object is duplicated before passing it to cache, correct? This results in 7 lines instead of 1 as demonstrated. Is there a better solution which will keep the code simple for developers?

  • Tom Lea

    Tom Lea August 13th, 2008 @ 06:51 PM

    As I understand it all Caches serialize (Martial or YAML) into the store, unless :raw is set. This is well tested and expected. Memory store should be no different.

    If we dup in and dup out for :raw (which is only expected to be able to cache strings anyway), we fix the mutable cached values issue. If we Martial none raw values, everything works as expected and fix #785.

    The next question is how the performance is v.s. other stores (mainly FileStore, it being the main competitor to MemoryStore in my mind) with and without martialing.


  • josh

    josh August 20th, 2008 @ 01:21 AM

    • State changed from “open” to “resolved”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href=""></a>


Referenced by