Friday, September 24, 2010

More on dealloc

I really didn't expect to kick off such a shitstorm yesterday with my dealloc post. A few people accused me of proselytizing the practice of nil'ing ivars, at least for release code. Possibly I did, but my real intention was to share the reasons why you might choose one approach over the other, not to say "you should do it this way." I mostly wrote the blog post to make sure I had my head fully wrapped around the debate because it had come up during the revising of Beginning iPhone Development.

Daniel Jalkut responded with a very lucid write-up that represents one of the common points of view on this matter. That view might best be summed up as "crash, baby, crash". If you want to find bugs, the best thing you can do is crash when they happen, that way you know there's a problem and know to look for it. This is not a new idea. I seem to remember a system extension back in the old Mac System 6 or System 7 days that would actually cause your system to crash if your app did a certain bad thing, even if that bad thing didn't cause any noticeable problems in your app. I don't honestly remember the specifics, but it was something to do with the trapping mechanism, I think (anybody know what I'm talking about??). Of course, Apple didn't ship that extension to non-developers.

What makes this a difficult argument is that Daniel is absolutely right… sometimes. There are scenarios where crashing is much better than, say, data loss or data integrity loss. General rules are always problematic, but though I see his point, I'm sticking with "not crashing on customers" being a good rule generally… except when it's not.

In interest of full disclosure, there's actually a third dealloc pattern, albeit a distant third in popularity, but it has definitely become more popular than when I first head of it. In this approach, you assign your instance variable to a stack local variable, then nil the instance variable before releasing it, like so:

- (void)dealloc
{
    id fooTemp = foo;
    foo = nil;
    [fooTemp release];
}

In the release-then-nil approach, after you release, there's a tiny window between the release and the assignment of nil where, in multithreaded code, the invalid pointer could still be theoretically be used. This approach prevents that problem by nil'ing first. This point of view represents the opposite end of the spectrum from the one Daniel stated. In this approach, your goal is to code defensively and never let your app crash if you can help it, even in development. If you're a cautious programmer and believe in the Tao of Defensive Programming, then there you go - that's your approach to dealloc.

For me, personally (warning - I'm about to state an opinion) I can't justify the extra code and time of the defensive approach. It's preemptive treatment of a problem so rare that it's almost silly that this discussion is even happening. I've been writing Objective-C since 1999 and I've only once seen a scenario where the dealloc approach would've made a difference in the actual behavior of the application, and that was a scenario created for a debugging exercise in a workshop, so we're really splitting hairs here.

So, here's my final word on dealloc:
  1. If you already have a strong opinion on which one to use for the type of coding you do, use that one. If not…
  2. If you prefer that bugs always crash your app so you know about them, use the traditional release-only approach
  3. If you prefer not to crash if you can help it and prefer to find your bugs using other means, use the newer approach
  4. If you want your app to crash for you, but not your customers, use the MCRelease() macro or use #if debug pre-processor directives
What's important is that you understand the benefits and problems of the different approaches, not that you use the same one that I do (or Daniel does, or anybody else does).

After all, you're the one who has to live with the consequences of your decision.



20 comments:

Brad Koehn said...

You mean EvenBetterBusError, which would make $00 point to an odd address in unaddressable memory, which would cause the machine to crash hard if you tried to dereference null. It also checked to make sure you didn't update it and would crash also. Great for debugging.

Uli Kusterer said...

Threading can always get in between two statements, so NILing the variable beforehand doesn't help with threading bugs (except make them a little bit rarer).

As to the "release-only approach" causing the app to "always crash". That's getting it backwards. The problem is not crashing bugs -- you'll always hear about crashes from your testers or users. The problem is that deallocated memory isn't zeroed out, so often you can keep manipulating an already-deallocated object, only to crash way later in a live object that got screwed up by the released one.

The reason for NILing in dealloc is not necessarily to keep the app from crashing, but rather to keep the app from crashing in useless and confusing ways.

DannyBVW said...

I think you are wrong in the way you nil your variables in your code. The big problem comes from the [object release], self.object = nil;

I am one to just use self.object = nil.

The reason behind this, is when you do [object release], self.object = nil;, you are actually running the risk of double releasing an object that is retained else where.

Think of the [self setObject]; method that @synthesize makes. It looks something similar to:

- (void)setObject:(ObjectClass *)theObject {
if (theObject != object) {
[object release];
object = theObject;
}
}

Of course, this is assuming that you are using @property (nonatomic, retain). I am not sure if this is exactly what it looks like, but the logic is there.

Essentially, every time that you call [object release], self.object = nil;, you are releasing the object twice. Most the time, this isn't a big deal, because the object usually only has a retain count of 1. But if the object is being retained elsewhere, you have just created the possibility of having pointers to memory that no longer exist, and getting a EXEC_BAD_ACCESS, causing a crash.

I do like doing a self.object = nil;, but I think by first releasing the object, then nil'ing it, you are creating a potential crash when other things reference the object.

Please correct me if I am wrong. I am not sure about the logic of setObject...

Andrew Lim said...

@DannyBVW - it was said in Jeff's first post about this...

Apple given advice:
"you should never, ever use mutators in dealloc (or init, for that matter)"

That and he is only doing "object = nil" which is a pure assignment.. big difference from "self.object = nil" which is as you have stated, actually a message call: "[self setObject:nil]"

Dan VanWinkle said...

@Andrew Thanks. I didn't see that he was just using object = nil. I had a co-worker doing the [object release], self.object = nil;, and it was causing crashes. I just ignored that he wasn't doing self.object. I don't see why it is so bad to use a mutator, like doing self.object = nil, considering that it is doing the exact same thing as [object release], object = nil;

Other than apple saying you shouldn't do it, is there a good reason not to?

Dave Murdock said...

@andrew @dan This link is the only thing I can find that says more than just bad idea: Avoiding KVO in dealloc

Dave Murdock said...

@uli still not sure if you are arguing for release-nil in both debug and release builds or only in release builds.

lksinc said...

Dan,

Nowhere does Jeff use that type of dealloc in his examples.

Jeff's code was doing:

[object release], object = nil;

which is not the same as:

[object release], self.object = nil;

In fact, I was thinking that this is the real other type that is used, at least I do this. My deallocs, simply have:

self.object = nil;

Which does the release and set to nil for me. Many people don't like this and say that there can be side effects (and there can), it's a similar argument for setting properties during init.

Normally your properties are synthesized so you don't need to bother, but you can override the setters and then you might get side effects. That should be part of your design considerations though when overriding a property setter.

Scott

danielpunkass said...

As Brad pointed out, EvenBetterBusError is probably the extension you are remembering. But there was also some cool runtime tweaking software, QC, that was a bit more hardcore than that.

David Birdsall said...

I really enjoy the healthy debate these types of posts generate; it just goes to show how tricky multithreaded development can be when accessing shared resources.

Perhaps the answer is to avoid (as much as possible) the sharing of objects between threads; or to apply the idea of object ownership not only to classes but to threads as well.

I wonder if this problem could be solved somehow through the use of Actors and shielding objects from one another, as described in Actor Thinking, using a library such as PLActorKit ?

Dad said...

Bravo! Well stated, clearly described pros and cons of different approach, open acknowledgement that there is not a "one true way" here and direction for people still finding their preferred approach.

DadGuy said...

I'd love to know as well the whys on not using property notation (mutators) to release objects in dealloc. I'd expect that to be consistent, if I am using property notation to set, say, a string, it would be "better code" to release it using the same property notation. I've done some looking and I can't find any reasoning that isn't subjective, similar to the other arguments that are being discussed here. Having been on the opposite end of this (not using the property notation on set and then double-releasing at dealloc time) I can see it going either way. Other than "you can do bad things" is there a more specific reason here?

mike3k said...

I believe the reason to avoid dot notation in dealloc is KVO. When you use a setter, it will send valueWillChange & valueDidChange to any observers, which could cause bad things to happen. If you don't have any observers, it *probably* won't be an issue.

QC was awesome. In addition to the EvenBetterBusError functionality it could also check all parameters for toolbox traps (which would slow the system down to a crawl).

DadGuy said...

mike,
at that point shouldn't observers be un-attached anyway? It was my understanding that observers had to be removed before an object was cleaned up or bad things would happen anyhow? Is this not always the case?

TheBarcodeProject said...

Not using mutators in dealloc is probably to do with KVO, the other side of the @property that is used for mutators.

When setting an object as nil in a dealloc, you're doing several things wrong:

1) You're promising observers access to an object currently in deallocation. That is dangerous say if there is a subclass of your object, which has already deallocated its objects before the superclass does, and the observer then goes to access that object's instance variables through a method, and it accesses stuff already deallocated as its in a superclass.

2) You're creating more work in a common method.

3) You're notifying observers that values have changed in an object when they haven't. If the "transaction.amount" is set to nil before it is deallocated, the user can then access those values as they are observed and display information incorrect for that object. The value never changes. The object is simply taken out of memory. Any storage of state in a databasing tool will therefore hold different information to the last notification presented by KVO. KVO is designed only to notify only when an property's value CHANGES, NOT when it is simply brought into and out of memory.

Martin said...

Idea:
Instead of setting the pointer to nil, you set it to pointer a static instance of an object that dumps on every message.
That way, you can catch more bugs. No more "it's pointing to something that seems to be valid", no more "hey, I'm sending a message to something that became nil and I never noticed". And you can only instantiate the static object in debug builds, and avoid crashes in release builds (or log, or something).

David Hart said...

I already asked this question in the previous post, but I'm asking it again here because I'm really curious as to getting an answer.

Can this macro the article is talking about be used to release IBOutlets? Or do they have to be nil-ed out, whatever the configuration? I'm wondering if it could cause any problems if used in viewDidUnload.

assur said...

http://www.sotobuy.com/
Jordan Shoes
jordan shoes,michael jordan shoes,air jordan shoes and brand shoes on sale.
http://www.sotobuy.com/ : Jordan Shoes

http://www.shoespuma.us/
Puma Shoes
puma shoes | puma running shoes | wholesale cheap puma shoes.
http://www.shoespuma.us/ : Puma Shoes

http://www.guccishoessupply.us/
Gucci Shoes
gucci shoes | discount gucci shoes and handbags on sale.
http://www.guccishoessupply.us/ : Gucci Shoes

http://www.monclerjacketstore.com/
Moncler Jackets
moncler|moncler jackets on sale,buy cheap moncler down jackets from monclerjacketstore.com
http://www.monclerjacketstore.com/ : Moncler Jackets

http://www.christianlouboutin2buy.com/
Christian Louboutin
christian louboutin,christian louboutin shoes,christian louboutin boots sale.
http://www.christianlouboutin2buy.com/ : Christian Louboutin

http://www.0595b.com/
NFL Jerseys
wholesale discount-nfl jerseys,mlb jerseys,nhl jerseys,sport jerseys shop.
http://www.0595b.com/ : NFL Jerseys

http://www.ghdhairstraighteners.cc/
GHD Hair Straighteners
ghd hair straighteners, cheap ghd straighteners, ghd-chi flat irons
http://www.ghdhairstraighteners.cc/ : GHD Hair Straighteners

Web Development Company said...

Nice post. Thanks for sharing nice information. It is very useful for iphone development and web development companies. Awesome post.

SEO Services Consultants said...

Nice information, many thanks to the author. It is incomprehensible to me now, but in general, the usefulness and significance is overwhelming. Thanks again and good luck! Web Design Company