Thursday, September 23, 2010

Dealloc

Last week there was a bit of a Twitter in-fight in the iOS community over the "right" way to release your instance variables in dealloc. I think Rob actually started it, to be honest, but I probably shouldn't be bringing that up.

Basically, several developers were claiming that there's never a reason to set an instance variable to nil in dealloc, while others were arguing that you should always do so.

To me, there didn't seem to be a clear and compelling winner between the two approaches. I've used both in my career. However, since we're in the process of trying to decide which approach to use in the next edition of Beginning iPhone Development, I reached out to Apple's Developer Tools Evangelist, Michael Jurewitz, to see if there was an official or recommended approach to handling instance variables in dealloc.

Other than the fact that you should never, ever use mutators in dealloc (or init, for that matter), Apple does not have an official recommendation on the subject.

However, Michael and Matt Drance of Bookhouse Software and a former Evangelist himself, had discussed this issue extensively last week. They kindly shared their conclusions with me and said it was okay for me to turn it into a blog post. So, here it is. Hopefully, I've captured everything correctly.

The Two Major Approachs

Just to make sure we're all on the same page, let's look at the two approaches that made up the two different sides of the argument last week.

Just Release

The more traditional approach is to simply release your instance variables and leave them pointing to the released (and potentially deallocated) object, like so:
- (void)dealloc
{
    [Sneezy release];
    [Sleepy release];
    [Dopey release];
    [Doc release];
    [Happy release];
    [Bashful release];
    [Grumpy release];
    [super dealloc];
}
In this approach, each of the pointers will be pointing to a potentialy invalid object for a very short period of time — until the method returns — at which point the instance variable will disappear along with its owning object. In a single-threaded application (the pointer would have to be accessed by something triggered by code in either this object's implementation of dealloc or in the dealloc of one of its superclasses, there's very little chance that the instance variables will be used before they go away , which is probably what has led to the proclamations made by several that there's "no value in setting instance variables to nil" in dealloc.

In a multi-threaded environment, however, there is a very real possibility that the pointer will be accessed between the time that it is deallocated and the time that its object is done being deallocated. Generally speaking, this is only going to happen if you've got a bug elsewhere in your code, but let's face it, you may very well. Anyone who codes on the assumption that all of their code is perfect is begging for a smackdown, and Xcode's just biding its time waiting for the opportunity.

Release and nil

In the last few years, another approach to dealloc has become more common. In this approach, you release your instance variable and then immediately set them to nil before releasing the next instance variable. It's common to actually put the release and the assignment to nil on the same line separated by a comma rather than on consecutive lines separated by semicolons, though that's purely stylistic and has no affect on the way the code is compiled. Here's what our previous dealloc method might look like using this approach:

- (void)dealloc
{
    [sneezy release], sneezy = nil;
    [sleepy release], sleepy = nil;
    [dopey release], dopey = nil;
    [doc release], doc = nil;
    [happy release], happy = nil;
    [bashful release], bashful = nil;
    [grumpy release], grumpy = nil;
    [super dealloc];
}
In this case, if some piece of code accesses a pointer between the time that dealloc begins and the object is actually deallocated, it will almost certainly fail gracefully because sending messages to nil is perfectly okay in Objective-C. However, you're doing a tiny bit of extra work by assigning nil to a bunch of pointers that are going to go away momentarily, and you're creating a little bit of extra typing for yourself in every class.

The Showdown

So, here's the real truth of the matter: The vast majority of the time, it's not going to make any noticeable difference whatsoever. If you're not accessing instance variables that have been released, there's simply not going to be any difference in the behavior between the two approaches. If you are, however, then the question is: what do you want to happen when your code does that bad thing?

In the first approach, your application will usually crash with an EXC_BAD_ACCESS, though you could also end up with any manner of odd behavior (which we call a "heisenbug") if the released object is deallocated and its memory is then reused for another object owned by your application. In those cases, you may get a selector not recognized exception when the message is sent to the deallocated object, or you may simply get unexpected behavior from the same method being called on the wrong object.

In the other approach, your application will quietly send a message to nil and go about its merry way, generally without a crash or any other immediately identifiable problem.

The former approach is actually good when you're developing, debugging, and doing unit testing, because it makes it easier to find your problematic code. On the other hand, that approach is really, really bad in a shipping application because you really don't want to crash on your users if you can avoid it.

The latter approach, conversely, can hide bugs during development, but handles those bugs more gracefully when they happen, and you're far less likely to have your application go up in a big ball of fire in front of your users.

The Winner?

There really isn't a clear cut winner, which is probably why Apple doesn't have an official recommendation or stance. During their discussion, Matt and Michael came up with a "best of both worlds" solution, but it requires a fair bit of extra code over either of the common approaches.

If you want your application to crash when a released pointer is accessed during development and debugging, the solution is to use the traditional approach in your debug configuration. If you want your application to degrade gracefully for your users, the solution is to use the newer approach in your release and ad hoc configurations.

One somewhat pedantic implementation of this approach would be this:

- (void)dealloc
{
#if DEBUG
    [Sneezy release];
    [Sleepy release];
    [Dopey release];
    [Doc release];
    [Happy release];
    [Bashful release];
    [Grumpy release];

    [super dealloc];
#else
    [sneezy release], sneezy = nil;
    [sleepy release], sleepy = nil;
    [dopey release], dopey = nil;
    [doc release], doc = nil;
    [happy release], happy = nil;
    [bashful release], bashful = nil;
    [grumpy release], grumpy = nil;

    [super dealloc];
#endif
}

That code assumes that your debug configuration has a precompiler definition of DEBUG, which you usually have to add to your Xcode project - most Xcode project templates do not provide it for you. There are several ways you can add it, but I typically just use the Preprocessor Macros setting in the project's Build Configuration:
Screen shot 2010-09-23 at 7.56.28 PM.png

Although the code above does, indeed, give us the best of both worlds - a crash during development and debugging and graceful degrading for customers - it at least doubles the amount of code we have to write in every class. We can do better than that, though. How about a little macro magic? If we add the following macro to our project's .pch file:

#if DEBUG
#define MCRelease(x) [x release]
#else
#define MCRelease(x) [x release], x = nil
#endif


We can then use that macro in dealloc, and our best-of-both-worlds code becomes much shorter and more readable:

- (void)dealloc 
{

    MCRelease(sneezy);
    MCRelease(sleepy);
    MCRelease(dopey);
    MCRelease(doc);
    MCRelease(happy);
    MCRelease(bashful);
    MCRelease(grumpy);

    [super dealloc];
}


Once you've got the macro in your project, this option is actually no more work or typing than either of the other dealloc methods.

But, you know what? If you want to keep doing it the way you've always done it, it's really fine, regardless of which way you do it. If you're consistent in your use and are aware of the tradeoffs, there's really no compelling reason to use one over the other outside of personal preference.

So, in other words, it's kind of a silly thing for us all to argue over, especially when there's already politics, religions, and sports to fill that role.

The Garbage Collection Angle

There's one last point I want to address. I've heard a few times from different people that setting an instance variable to nil in dealloc acts as a hint to the garbage collector when you're using the allowed-not-required GC option (when the required option is being used, dealloc isn't even called, finalize is). If this were true, forward compatibility would be another possible argument for preferring the newer approach to dealloc over the traditional approach.

While it is true that in Java and some other languages with garbage collection, nulling out a pointer that you're done with helps the garbage collector know that you're done with that object, so it's not altogether unlikely that Objective-C's garbage collector does the same thing, however any benefit to nil'ing out instance variables once we get garbage collection in iOS would be marginal at best. I haven't been able to find anything authoritative that supports this claim, but even if it's 100% true, it seems likely that when the owning object is deallocated a few instructions later, the garbage collector will realize that the deallocated object is done with anything it was using.

If there is a benefit in this scenario, which seems unlikely, it seems like it would be such a tiny difference that it wouldn't even make sense to factor it into your decision. That being said, I have no firm evidence one way or the other on this issue and would welcome being enlightened.

After writing this, Michael Jurewitz pinged me to confirmed that there's absolutely no benefit in terms of GC to releasing in dealloc, though it is otherwise a good idea to nil pointers you're done with under GC..

Thanks to @borkware and @eridius for pointing out some issues

Update: Don't miss Daniel Jalkut's excellent response.



39 comments:

Eridius said...

Even in a single-threaded app, released ivars in dealloc can still be accessed again if super's dealloc calls any methods on self. I have actually run into this situation myself, which is why I'm firmly on the side of nilling out ivars.

On another note, not nilling out the ivar in an express attempt to cause a crash if it's reaccessed is reasonable, but if this is your desired behavior it would be far better to actually set the ivar to a garbage pointer. However, you probably don't want to do this if zombies are enabled. So you may want to make your macro look something like

#if DEBUG
#define MCRelease(x) do { [x release]; if (!NSZombieEnabled) x = 0xDEADBEEF; } while (0)
#else
#define MCRelease(x) [x release], x = nil
#endif

Michael said...

Jeff, thanks for the write up.

When I first started coding for iOS (only 2 years ago) I used the traditional approach because that is what I saw in Apple's examples.

More recent examples seem to have adopted the modern approach - OR is that just to do with nil'ing IBOutlets and should that be done as a matter of course anyway??

Either way your macro approach seems the best solution going forward.

Thanks again, Michael.

Les Cop's de Sam said...

Great post! I discovered the Macro approach after looking into Three20 code.

Daniel said...

In a multithreaded app, even setting the ivar to nil does not protect against EXC_BAD_ACCESSES, since a second thread can access the dead object between the release message and the ivar assignment. So, really, you're just making the heisenbug less likely but even harder to track down when it does happen.

@Michael: I think the pattern you're referring to is setting IBOutlet properties to nil on viewDidUnload. This is a good idea because you'll end up retaining unnecessary objects that will be recreated when the view is reloaded from the NIB anyways.

Jeff LaMarche said...

Daniel:

Not to argue semantics, but I'd say it definitely protects against EXC_BAD_ACCESS, it just doesn't insure you won't get them. But, yes, you're right, there is still a tiny window during which that could happen, but it's a considerably smaller window than if you don't do it at all, especially in a class with a lot of ivars.

I suppose you could @synchronize the release and nil assignment and use the same semaphore you use in your accessor and mutator, but I think you're probably past the point of diminishing returns at that point.

sweller said...

Don't forget to nil out delegate pointers to self as well.

alice.delegate = nil, [alice release], alice = nil;

Your delegates can have lifetimes that extend beyond what you expect: timers, objects that retain their delegates (NSURLConnection for example), delayed message sends, programming errors, and plain old correct code that you have forgotten that retains the delegate.

I'm a firm believer in nilling out ivars, having seen many instances where the old object pointers have caused all manner of debugging fun. Had they been nilled, the effects would have been benign.

I do like the idea of using a bad pointer instead of nil for debug builds. But how about creating an object (a subclass of NSProxy?) with no methods and using a pointer to that instead? You'll get more information when the debugger stops on the runtime exception than you would with a bad access.

Daniel said...

@Jeff: You're right; it does protect against it, but doesn't guarantee you won't get bitten (I guess that's what I meant). But, now that I think about it, if you have a thread accessing ivars of a dealloc'ed object, wouldn't you crash and burn pretty quickly anyways?

Jeff LaMarche said...

Daniel:

My experience with mistakes in thread like that is that they're unpredictable as all hell, but in most cases, they probably would crash pretty quickly.

Matt said...

I've been using the macro approach for about a year, and I haven't looked back. It's less to write, and it just feels right (to me).

Nick Forge said...

@sweller: that's exactly what I was thinking. Using some sort of global object that responds with a console message when it is messaged would help debug these issues.

If you're going to try and induce a crash (to expose a bug), you may as well do it in a way that gives you some helpful debugging information.

danielpunkass said...

Good rundown on the arguments, Jeff. I think you give too much credence to the nil'ing approach, and I want to try to talk you out of doing it.

http://www.red-sweater.com/blog/1423/dont-coddle-your-code

Darren Stone said...

I can't help but shake my head whenever these arguments against basic, fundamental defensive programming practices crop up.

First, your superclass is allowed to call methods in its deallocator, methods that you may have overridden. That isn't a bug, but it does need to be handled correctly, so you'd better make sure your class is always in a consistent state before you hand off control to another method, like [super dealloc].

Second, the argument for leaving a bunch of dangling pointers in your deallocator is that if they are accessed again it will trigger a crash that you can debug.

That's wrong. Here's why:

Who knows what the retain counts are at the time of release? The pointers may or may not be invalid, may or may not trigger a crash, and that could depend on a whole bunch of factors beyond your control.

If you leave those pointers dangling you are just asking for a crash in production code, triggered by the phase of the moon, that is very costly and time consuming to investigate. And when you do finally discover the cause the of the crash, you'd likely find that the lowest-impact fix to that production code is to just nil the pointer in your deallocator! We are Obj-C developers, after all.

Consider this example:

1. In your deallocator, you leave the ivar _foo dangling, but the instance in _foo is also referenced by some other external code, so the instance is still alive, the pointer still valid.

2. Your superclass deallocator triggers this code through an overridden method:

[_foo release]; _foo = nil; // Does this look like a bug to you?

3. There's no crash, yet, but you've just screwed that external code that is also holding a reference to _foo. Now when that external code runs it will crash, and your code is nowhere to be found in the call stack.

How would you like to be responsible for debugging that little number?

charles said...

@sweller and @nickforger: is it not pretty much equivalent to using NSZombieEnabled?

danielpunkass said...

Darren - I'm not sure I completely follow your example, but the "dangling pointer" you allude to is not dangerous to "external code" unless it is failing to fulfill its memory management contract and retaining the object for as long as it needs it.

You seem to suggest that a superclass implementation of dealloc might release _foo as well. This is just a bug. It should never be the case that a base class and a subclass both take responsibility for releasing an instance variable. The subclass should only be releasing it because it added the instance variable to the class.

Dave Murdock said...

Think this tweet from Uli Kusterer got lost in the stream, but it's a good one: https://twitter.com/uliwitness/status/25370226517

Here's the macro:

// The do/while(0) stuff is just there so the macro behaves just like any other
// function call, as far as if/else etc. are concerned.
#define DESTROY(targ) do {\
NSObject* __UKHELPERMACRO_OLDTARG = (NSObject*)(targ);\
(targ) = nil;\
[__UKHELPERMACRO_OLDTARG release];\
} while(0)

Seems like this would totally close the extremely small potential window on the heisenbug.

Any reason this isn't the best approach for release builds if you want to be as defensive as possible?

Jeff Laing said...

No-one else seems to have pointed out the brilliant demonstration of doing something completely different #if DEBUG.

Its obviously a typo, but you release a completely different set of instance variables in the debug case.

(check the upper-casing - Sneezy vs sneezy)

digdog said...

Here's my thought about nil in -dealloc:

http://digdog.tumblr.com/post/1177480301/to-nil-or-not-to-nil-that-is-the-question

Daniel said...

Uli Kusterer's macro looks like the ideal solution. That one does actually guarantee thread-safety, though perhaps there needs to be a memory barrier between setting nil and releasing?

Darren Stone said...

@ danielpunkass

Yes, it is a bug; where exactly depends on the design. The point is that it's a bug that's very difficult and costly to diagnose and fix.

This kind of defensive programming is not about hiding bugs, it's about preventing particularly nasty, expensive bugs from occurring.

Michal said...


Other than the fact that you should never, ever use mutators in dealloc (or init, for that matter) ....


Well, this is not always true. If you are using the modern runtime and synthesizing the instance variable, however, you cannot access the instance variable directly, so you must invoke the accessor method.

http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/ObjectiveC/Articles/ocProperties.html%23//apple_ref/doc/uid/TP30001163-CH17-SW16

Of course this can have side effects if you're using custom setter that does something more as the generated one.

David Hart said...

Nice modification Eridius.
I have two questions though:

1) Can we use this macro for releasing properties?
2) If the above is true, can it also be used in viewDidUnload?

Jerm said...

Thanks as usual for the clarification Jeff.

Another useful one for newbies could be equivalent guidelines for initialisation, for ivars with and without @property specifications of each different type (retain, copy assign etc.)

jr said...

Just a quick question. If it is important to nil the object why doesn't release actually do the nil?

Daniel said...

@Michal: I think that bit of documentation is wrong; it definitely is possible to directly access synthesized ivars. In some earlier versions of Xcode/GCC (during the 10.5 era), you couldn't access synthesized ivars, but this was considered a bug in GCC.

I'm trying to find a definitive reference for that, but the closest I can find is a StackOverflow answer that also contains no reference. I could've sworn there was an Apple rep stating this on cocoa-dev, but I can't find it now.

Dave Murdock said...

@Daniel Documentation is wrong on ivars being inaccessible, but you still conclude this is right?

[self setIVar:nil]

I am not saying it isn't, I have release, release & nil, and set synthesized ivar to nil using accessor in dealloc, sometimes all in the same dealloc!

Looking to clean this up and don't think we have reached a conclusion on "best".

Daniel said...

@Dave: Actually, I personally always just directly release and leave dangling pointers. If there are threading issues, those are pretty serious problems and just doing the nil thing in dealloc isn't really going to save you. And, in practice, if you test with NSZombiesEnabled it's pretty obvious if [super dealloc] calls something that accesses a released ivar.

However, if you do want to set to nil, I think the best is your previous comment where you assign the ivar to nil first, then release.

Steve said...

@Daniel: Uli's macro absolutely needs a memory barrier to stop both the compiler from reordering the logic at build time, and to stop the CPU from executing instructions out-of-order at runtime.

bfl said...

I think that Eridius was onto something. I would only modify it to make each release ivar have a unique pointer so you can figure out which one has the stale reference from the crash log.

Karl said...

If you're really worried about [super dealloc] calling methods that you've overridden, you can always use autorelease instead of release.
Mind you, you'll want to be careful with objects that allocate large amounts of memory if you do this.

I'd rather see a program crash than get bad data. If a program crashes, you can just restart it. If a program gets bad data, you could be in for a world of hurt.

For example:

- (void) saveUserData:(AllData*) allData
{
...
[[DataPersistence sharedInstance] persistUserLevel:allData.userData.level];
...
}

Now suppose userData were deallocated inside AllData's dealloc, and saveUserData just happened to be called in a separate thread during AllData's dealloc.

What would happen if userData had been released and destroyed? The app would likely crash with EXC_BAD_ACCESS and a stack trace pointing to saveUserData as the culprit. Since this is a multithreaded app, it wouldn't take too long to figure out that you'd accessed a deallocated object in a race condition.

Now what would happen if userData had been set to nil afterwards and saveUserData accessed it after that point? The app would happily persist 0 as the user level, corrupting the save data. So now instead of a user who's annoyed at a crash, you have a user who's furious that your game clobbered his save data after he spent weeks getting to level 50!

Now what would happen if userData had been released and destroyed AND something else was allocated in its place? We'd be in a similar "bad data" situation, of course. But how likely is it for another object to be allocated starting on the exact same address? Furthermore, how likely is it that an object which responds to the "level" selector happens to be placed in exactly that address? The chances of this occurring are orders of magnitude smaller than getting 0 back from a selector call to nil.

There's a reason why uncaught exceptions crash the app, and that reasoning is that it's better to shut down the program than to allow it to behave unpredictably and corrupt data. If you really do believe that setting to nil in dealloc to force nil behavior rather than crash is the best approach, then you should also be in favor of having uncaught exceptions simply restore the stack frame to the point of the exception and let the app continue as if you were ignoring a return value in C.

Stelian said...

While using the macro from Eridius, I get warnings like

warning: incompatible integer to pointer conversion assigning to 'NSArray *' from 'unsigned int'

Any ideas how to get rid of those?

Elliot said...

Other than the fact that you should never, ever use mutators in dealloc (or init, for that matter) ....

Why not?

Elliot said...

Eridius's macro doesn't quite work for me:

error: 'NSZombieEnabled' undeclared (first use in this function)

... or was that meant to be pseudocode? (In which case, actual code would be nice...)

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

Niklas Björnberg said...

Actually, I think that the abaility for ObjC to accept messages to nil is bad. If there is a nil reference being sent a message, there's proabbly an error in the code somewhere, and I'd like to know about it as fast and blatantly as possible. :)

Niklas Björnberg said...

errata: ability, probably.

cheers!

Hire iphone developer said...

Vary nice blog I like it,
Thanks for sharing a wonderful information.
hire iphone developer

Anders said...

Just a side note: When it comes to the "#if DEBUG" line, it is never a good idea to have undefined symbols in preprocessor expressions, which is the case in the "release" configuration.

Instead, you should use one of the following patterns:

1) Define the symbol in one project configuration and use #ifdef.

2) Define the symbol as DEBUG=0 and DEBUG=1 in the release and debug configurations, and continue to use "#if".

David said...

I just looked at the latest copy of Apple's Memory Management Guide. Specifically for ivars, they just use release with no nil. However, for iOS 3.0 and earlier they SPECIFICALLY say to nil outlets:

In addition, because of a detail of the implementation of dealloc in UIViewController, you should also set outlet variables to nil in dealloc:
- (void)dealloc {
// Release outlets and set outlet variables to nil.
[anOutlet release], anOutlet = nil;
[super dealloc];
}

What this implies to me is that the nil is ONLY needed for older iOS releases, not 4.0 and newer.

Jeffrey Walton said...

It seems the DEBUG of the 'safe relase' macro is missing an awesome programming aid: the assert. Why debug code when you can let the code debug itself?

#define SAFE_OBJC_RELEASE(x) { ASSERT((x) != nil); if((x) != nil) { [(x) release], (x) = nil; } }

A solid assert strategy can save time, and catch the sloppiness of using Nil and NULL objects, freeing NULL pointers, malloc'ing for 0 size memory, bad unction parameters, unexpected results from functions, ...

Keep in mind that assert's are controlled via NDEBUG*, and not DEBUG. So make sure release builds #define NDEBUG.

Finally, SIGABORT is almost always useless behavior from an assert, so raise a SIGTRAP in ASSERT:

#define SFUASSERT(exp) \
{ \
if(!(exp)) \
{ \
fprintf(stderr, "Assertion failed: %s:%s(%d)\n", __FILE__, __func__, __LINE__); \
raise(SIGTRAP); \
} \
}

Jeffrey Walton
Baltimore, MD, US

* http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html