Ruby on Rails

Comments and changes to this ticket

• 

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

• 

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

  • no changes were found...

  • dup_in_memory_store.diff 2 KB
• 

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

• 

  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 August 11th, 2008 @ 07:07 PM

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

• 

  Joshua Peek 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”
  • → Tag changed from “activesupport bug patch” to “activesupport patch”
  • → Assigned user changed from “” to “Joshua Peek”

  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 August 11th, 2008 @ 09:52 PM

  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.

• 

  Joshua Peek 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 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 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.

  Thoughts?

• 

  Joshua Peek August 20th, 2008 @ 01:21 AM

  • → State changed from “open” to “resolved”

  http://github.com/rails/rails/co...