#1527 For ActiveRecord models there should be a way to avoid calling memoized methods when you call #destroy - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#1527 ✓stale

For ActiveRecord models there should be a way to avoid calling memoized methods when you call #destroy

Reported by Mike Grafton | December 8th, 2008 @ 03:08 AM | in 3.x

When you call #destroy on an AR model, it calls #freeze. The ActiveSupport::Memoizable mixin, however, alias_method_chains freeze to add a call to #memoize_all. This is probably not what you want on destroy (or maybe on freeze at all), since memoized methods are often computationally expensive.

If this is really the desired behavior, then there should be a way around it (there is currently not a good one). Either a :freeze => false option could be passed to destroy, or a :on_freeze => false could be passed to #memoize.

Comments and changes to this ticket

josh December 8th, 2008 @ 03:49 AM
- State changed from “new” to “open”
I think it makes sense that destroy would freeze an AR object because you should not able to make any changes after.

And one of the main reasons for freeze to invoke your memoized methods is to make sure the ivar is set since you can't modify them after. Otherwise you'd get an error when you called a memorized method on a frozen object.

So both have there reasons, however combining leads to the issue that you explained.

The temporary fixed would be to over the freeze_with_memoizable method.
```
def freeze_with_memoizable
  freeze_without_memoizable
end
```
As mentioned a possible API to exclude a method from would be "memoize :foo, :freeze => false" or something like that. But there are some other issues we still have to consider like does this apply to other methods on the object and what happens when you call that method again.

Heres my proposal: Create 2 separate cases for what will happen to each method on freeze.

1) Current behavior: On freeze, we prime the cache so the method will be permanently remembered and would work under the frozen context. Default setting.
```
memoize :foo, :on_freeze => :prime
# or just
memoize :foo
```
2) Alternate behavior: On freeze, discard the memoization feature. This way the method will still work (unmemoized) while frozen. I think the easiest way to accomplish this would be to alias the original method back the its original name.
```
memoize :foo, :on_freeze => :flush
```
Take a stab at it, and I'll help you out if you need some.
You flagged this item as spam.
Mike Grafton December 8th, 2008 @ 01:54 PM
Cool, thanks.

I actually think it might be better to throw an error if you called an un-primed memoized method after freeze. Especially if it blew up in an obvious way, with an explicit message like "This memoized method cannot be called after freeze. Use :on_freeze => :prime"

But I admit, I don't really understand freezing well enough to have a strong opinion. For my case your solution would work, so I'll give it a shot.
josh December 28th, 2008 @ 08:55 PM
- State changed from “open” to “incomplete”
You flagged this item as spam.
Andreas Neuhaus January 4th, 2009 @ 11:51 AM
The above workaround did not work for me (freeze was still calling memoize_all). However, the following worked for me:

def freeze freeze_without_memoization end
You flagged this item as spam.
ransom-briggs (at uiowa) July 9th, 2009 @ 04:00 PM
+1 for an :on_freeze => :flush
josh August 19th, 2009 @ 03:55 PM
- Assigned user cleared.
Jeremy Kemper May 4th, 2010 @ 06:48 PM
- Milestone changed from “2.x” to “3.x”
[bulk edit]
You flagged this item as spam.
jerrett November 4th, 2010 @ 06:23 PM
- Importance changed from “” to “”
I like the :flush idea as well, +1.
You flagged this item as spam.
Evgeniy Dolzhenko January 10th, 2011 @ 01:35 PM
If "one of the main reasons for freeze to invoke your memoized methods is to make sure the ivar is set since you can't modify them after" why don't we just create the required ivars on freeze without actually priming the cache (and calling memoized methods in turn) then?
You flagged this item as spam.
dspencer January 11th, 2011 @ 12:16 AM
I'm not sure I'm going about this in the right way; but I made a patch that saves a Set of methods to be primed on freeze; and only primes those.

I can't seem to attach a patch; pasted it to this gist instead.
https://gist.github.com/773768
You flagged this item as spam.
Evgeniy Dolzhenko January 11th, 2011 @ 07:56 AM
Really the "prime on freeze" idea makes little sense to me especially because memoization is arguments based so you'll be able to prime cache only for the methods which don't take any arguments.
So I can't see the need in these additional options to memoize, just create the required (empty) ivars on freeze which will be filled later when required (turns out freeze is not "deep" in ruby, i.e. you can't add instance variables on the frozen object, but can change them freely)
rails April 12th, 2011 @ 01:00 AM
- State changed from “incomplete” to “open”
This issue has been automatically marked as stale because it has not been commented on for at least three months.

The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.
rails April 12th, 2011 @ 01:00 AM
- State changed from “open” to “stale”