MemoryStore cache contents should be immutable

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 cache.read('foo')[:a] # => 2

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

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

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
    end
  end

  def test_instance_variable_duping
    instance = SimpleClass.new(SimpleClass.new("Pass"))
    @cache.write("instance", instance)
    instance.value.value = "FAIL!"
    assert_equal "Pass",  @cache.read("instance").value.value
  end

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

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

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

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

all_cached = Rails.cache.read('Recipe.all')
if all_cached.nil?
  all = Recipe.all
  Rails.cache.write('Recipe.all', all.dup.freeze)
else
  all = all_cached.dup
end

# 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.

@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.

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?

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 Caching models fails in development.

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.

Thoughts?