424 1 true 2008-08-11T16:00:05+01:00 7840 9903 799 memorystore-cache-contents-should-be-dup-d 260 8994 resolved activesupport patch 2008-08-20T01:21:41+01:00 424 Joshua Peek Jim Lindley Joshua Peek http://rails.lighthouseapp.com/projects/8994/tickets/799 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) <code> cache = ActiveSupport::Cache.lookup_store(:memory_store) h = {:a => 1} cache.write('foo', h) h[:a] = 2 cache.read('foo')[:a] # => 2 <code> 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). 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) <code> cache = ActiveSupport::Cache.lookup_store(:memory_store) h = {:a => 1} cache.write('foo', h) h[:a] = 2 cache.read('foo')[:a] # => 2 <code> 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). <div><p>This was a side issue to ticket <a href="/projects/8994/tickets/785" title="Ticket #785">#785</a>. Splitting out into it's own ticket for further discussion. MemoryStore cache contents can be altered by changing fetched objects outside of the cache.</p> <p>"[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 <a href="/projects/8994/tickets/785" title="Ticket #785">#785</a>)</p> <p>cache = ActiveSupport::Cache.lookup_store(:memory_store) h = {:a => 1} cache.write('foo', h) h[:a] = 2 cache.read('foo')[:a] # => 2</p> <p>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).</p></div> 0 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) @@@ ruby cache = ActiveSupport::Cache.lookup_store(:memory_store) h = {:a => 1} cache.write('foo', h) h[:a] = 2 cache.read('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). <div><p>This was a side issue to ticket <a href="/projects/8994/tickets/785" title="Ticket #785">#785</a>. Splitting out into it's own ticket for further discussion. MemoryStore cache contents can be altered by changing fetched objects outside of the cache.</p> <p>"[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 <a href="/projects/8994/tickets/785" title="Ticket #785">#785</a>)</p> <pre><code class="ruby"> cache = ActiveSupport::Cache.lookup_store(:memory_store) h = {:a =&gt; 1} cache.write('foo', h) h[:a] = 2 cache.read('foo')[:a] # =&gt; 2 </code></pre> <p>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).</p></div> false 2008-08-11T17:25:37+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport 2008-08-11T17:25:37+01:00 7840 Jim Lindley Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 0 Aaargh, getting the code formatting wrong. Trying again, with Ryan's example. Existing behaviour: @@@ ruby cache = ActiveSupport::Cache.lookup_store(:memory_store) h = {:a => 1} cache.write('foo', h) h[:a] = 2 cache.read('foo')[:a] # => 2 @@@ <div><p>Aaargh, getting the code formatting wrong. Trying again, with Ryan's example.</p> <p>Existing behaviour:</p> <pre><code class="ruby"> cache = ActiveSupport::Cache.lookup_store(:memory_store) h = {:a =&gt; 1} cache.write('foo', h) h[:a] = 2 cache.read('foo')[:a] # =&gt; 2 </code></pre></div> false 2008-08-11T16:02:16+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport 2008-08-11T16:02:16+01:00 7840 Jim Lindley Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 1 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. <div><p>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.</p> <p>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.</p></div> false 2008-08-11T16:23:41+01:00 7840 --- :tag: activesupport 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport bug patch 2008-08-11T16:23:41+01:00 5701 Ryan Bates Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 1 Doesn't look like I can edit the original. Wish there was a preview button. Stuck with monospaced. <div><p>Doesn't look like I can edit the original. Wish there was a preview button. Stuck with monospaced.</p></div> false 2008-08-11T16:34:01+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport bug patch 2008-08-11T16:34:01+01:00 7840 Jim Lindley Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 1 There should be an "Edit Ticket" link in the blue bar at the top. At least that's available on my tickets. <div><p>There should be an "Edit Ticket" link in the blue bar at the top. At least that's available on my tickets.</p></div> false 2008-08-11T16:51:17+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport bug patch 2008-08-11T16:51:17+01:00 5701 Ryan Bates Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 1 Not sure if this is a show stopper but the following test (inserted in MemoryStoreTest) fails: @@@ ruby 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. <div><p>Not sure if this is a show stopper but the following test (inserted in MemoryStoreTest) fails:</p> <pre><code class="ruby"> class SimpleClass attr_accessor :value def initialize(value) @value = value end end def test_instance_variable_duping instance = SimpleClass.new(SimpleClass.new(&quot;Pass&quot;)) @cache.write(&quot;instance&quot;, instance) instance.value.value = &quot;FAIL!&quot; assert_equal &quot;Pass&quot;, @cache.read(&quot;instance&quot;).value.value end </code></pre> <p>No simple way around this, but it could be an argument in favour of serializing it despite the overhead.</p></div> false 2008-08-11T16:57:08+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport bug patch 2008-08-11T16:57:08+01:00 28420 Tom Lea Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 1 Ryan, I get those edit links on my own Lighthouse projects, but not when I'm logged in here at the Rails project. <div><p>Ryan, I get those edit links on my own Lighthouse projects, but not when I'm logged in here at the Rails project.</p></div> false 2008-08-11T17:04:31+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport bug patch 2008-08-11T17:04:31+01:00 7840 Jim Lindley Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 1 Fixed thanks to Rick and Will - the edit link shows up for me now. <div><p>Fixed thanks to Rick and Will - the edit link shows up for me now.</p></div> false 2008-08-11T19:07:32+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 new activesupport bug patch 2008-08-11T19:07:32+01:00 7840 Jim Lindley Jim Lindley http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 424 1 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. <div><p>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.</p></div> false 2008-08-11T21:01:50+01:00 7840 --- :tag: activesupport bug patch :state: new :assigned_user: :title: MemoryStore cache contents should be dup'd.. 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 open activesupport patch 2008-08-11T21:01:50+01:00 424 Joshua Peek Jim Lindley Joshua Peek http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 424 1 Requiring all objects sent to cache be immutable can really complicate the code. For example: @@@ ruby 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. <div><p>Requiring all objects sent to cache be immutable can really complicate the code. For example:</p> <pre><code class="ruby"> 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 } </code></pre> <p>To me this seems like something the framework should do, especially considering we have the convenient "fetch" method built in.</p></div> false 2008-08-11T21:52:01+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 open activesupport patch 2008-08-11T21:52:01+01:00 5701 Ryan Bates Jim Lindley Joshua Peek http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 424 1 @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. <div><p>@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 :)</p> <p>You never want to modify objects in the cache store.</p></div> false 2008-08-13T01:45:23+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 open activesupport patch 2008-08-13T01:45:23+01:00 424 Joshua Peek Jim Lindley Joshua Peek http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 424 1 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? <div><p>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?</p></div> false 2008-08-13T17:45:03+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 open activesupport patch 2008-08-13T17:45:03+01:00 5701 Ryan Bates Jim Lindley Joshua Peek http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 424 1 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? <div><p>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.</p> <p>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 <a href="/projects/8994/tickets/785" title="Ticket #785">#785</a>.</p> <p>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.</p> <p>Thoughts?</p></div> false 2008-08-13T18:51:16+01:00 7840 --- {} 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 open activesupport patch 2008-08-13T18:51:16+01:00 28420 Tom Lea Jim Lindley Joshua Peek http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x 424 1 http://github.com/rails/rails/commit/5de340e79f1d11973b7c7bbec82f320fc92b9c99 <div><p><a href="http://github.com/rails/rails/commit/5de340e79f1d11973b7c7bbec82f320fc92b9c99">http://github.com/rails/rails/co...</a></p></div> true 2008-08-20T01:21:41+01:00 7840 --- :state: open 9903 799 memorystore-cache-contents-should-be-dup-d 0 8994 resolved activesupport patch 2008-08-20T01:21:41+01:00 424 Joshua Peek Jim Lindley Joshua Peek http://rails.lighthouseapp.com/projects/8994/tickets/799 2.x fbf2ea1cc340811df4cca1838f04527ca2f0095e text/plain 2008-08-11T16:23:40+01:00 dup_in_memory_store.diff 38400 2003 5701 http://rails.lighthouseapp.com/attachments/38400/dup_in_memory_store.diff