Re: Cleanup inside a failed init method
Re: Cleanup inside a failed init method
- Subject: Re: Cleanup inside a failed init method
- From: "John Engelhart" <email@hidden>
- Date: Sun, 7 Dec 2008 00:27:29 -0500
On Sat, Dec 6, 2008 at 4:07 PM, Ken Tozier <email@hidden> wrote:
> Hi
>
> I'm writing my own socket class using a bunch of BSD functions and am a
> little unclear on exactly what I should be doing to insure everything is
> cleaned up if any of the low level functions fail. If I return nil from my
> init, does the system call my dealloc method to allow proper cleanup? Or do
> I have to do the cleanup before returning?
Within an -init method, it's best to assume that the memory allocated
for the object will persist forever (I'm assuming we're talking about
manual memory management here, not GC), and 'some kind' of action is
required to prevent a leak. This generally means you're going to have
to do a '[self release]' (or equivalent) to make sure that the objects
memory is properly reclaimed by the system. The authoritative
reference on the subject is:
http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/MemoryMgmt.html
Everyone seems to have their own style, so here's mine for what it's worth.
Consolidate all your clean-up code in one place, and one place only:
dealloc. I can't even think of a reasonable example where your
dealloc code couldn't figure things out just from the contents of
iVars. When allocated, your objects memory is zero'd, so you start
from a known state. If a pointer is non-NULL, then something probably
has to be done with it. Keeping everything in one place means there's
only one place you need to make changes, not two or three different
places, one of which Murphy will guarantee to not be up to date with
your latest tweaks.
When something goes wrong, you're eventually going to have to do the following:
[self release];
return(NULL);
Don't call -dealloc directly, EVER. From the NSObject -dealloc documentation:
You never send a dealloc message directly. Instead, an object's
dealloc method is invoked indirectly through the release NSObject
protocol method (if the release message results in the receiver's
retain count becoming 0).
When you call [self dealloc] directly from your -init... method,
you'll break a Cocoa memory management invariant: the objects
reference count is still > 0 at that point.
The trick is, of course, to handle all the cases correctly. Sometimes
there's 'hidden' or non-obvious ways that your init code could
potentially leak. A prime example is objc exceptions. If, for
example, in your -init.. code you have an NSArray, but accidentally
feed it a bogus index, it will throw a NSRangeException. In general,
unless you're inside a @try/@catch (or equivalent) block, control will
not return to your -init... method.
One pattern I've used for when things get 'complicated' like this, or
there's just too many ways that things could potentially go wrong, I
do something along the lines of:
- (id)init...
{
if((self = [super iniit...]) == NULL) { return(NULL); }
[self autorelease];
// Do hairy initialization bits here.
onSuccess:
return([self retain]);
}
This of course assumes that your -dealloc method will be able to mop
up any partial initialization. I've found this to be trivial in
practice. When something goes wrong in your -init... code, all you do
is immediately return(NULL); When the autorelease pool is popped
latter, your objects reference count will drop to zero, and your
-dealloc method will be called to clean things up, and the memory for
the object will be reclaimed.
The beauty of this approach is just about anything can go wrong in the
middle. Even if some object deep in the initialization logic throws
an exception which you weren't prepared for, your object is already
pushed on to the autorelease pool and is pretty much guaranteed to
have -dealloc called at some later point. The only time your object
won't be reclaimed is when you've successfully initialized everything
without an error and 'rescue' yourself from the clutches of the
autorelease pool. I've found this to drastically simplify complicated
-init... code where it's critical that the -init... method be very
robust and recover gracefully and correctly no matter what happens.
> Here's what I have so far
>
> - (id) initWithHost:(NSString *) inHost
> port:(int) inPort
> error:(NSString **) inError
> {
> id result = nil;
>
> self = [super init];
> if (self)
> {
> host = [inHost copy];
> port = inPort;
>
> // if any of the following fails, I want to cleanup
> // completely before returning
> // am I doing that correctly?
> // "host" and "error" are the only Cocoa objects
> // I'm holding on to. The other stuff is from the BSD
> functions
> if ([self getAddress] &&
> [self createSocket] &&
> [self connect] &&
> [self updateStream])
> {
> result = self;
> }
> else
> {
> freeaddrinfo(addressInfo);
> close(socketFD);
> *inError = [error copy];
> [error release];
> [host release];
> }
> }
>
> return result;
> }
Here's a few problems I see right away:
On an error, you leak the memory for the object. Your return nil, but
you never do a [self release], so that memory will remain allocated
until the end of time.
The lines
---
*inError = [error copy];
[error release];
---
will likely result in a crash when whoever called the init... method
attempts to use the error object. This is because you used -release,
which immediatly releases the object. After that release statement,
the object is 'gone', or at least should be treated as if it were gone
because you released your reference to it, and it's free to -dealloc
right away if the reference count dropped to 0. This is essentially
'Use after free()'. You should use [error autorelease] instead, which
registers a -release call with the top most autorelease pool. The
object remains live until that autorelease pool is popped, at which
point the autorelease pool sends all the release requests it received.
This allows the object to remain live for a (generally) brief period
of time, giving anyone of interest (which is almost always the caller
of the method) to send a -retain message to keep the object live if
need be.
Also, you don't check if inError != NULL, which will cause a crash
unless a valid pointer is passed in.
---
*inError = [[error copy] autorelease]; // Typical Cocoa way of doing things.
---
I'm assuming that most of these variables that haven't been explicitly
declared are iVars. One problem with the approach you've taken is
that I'm sure you have a -dealloc method which does something like:
- (void)dealloc
{
[host release];
// Likely the other cleanup parts in the init method above, too.
[super dealloc]
}
Assuming that the leak problem is fixed, you then run in to the
problem that, in all likely hood, -dealloc is going to perform the
exact same work again because none of the variables were cleared,
probably resulting in a crash.
Here's how I might approach it using the 'autorelease inside -init..'
technique I described above:
- (id) initWithHost:(NSString *)inHost port:(int)inPort
error:(NSString **) inError
{
if((self = [[super init] autorelease]) == NULL) { return(nil); }
if(inHost == nil) { [NSException raise:NSInvalidArgumentException
format:@"inHost == nil"]; }
if(inError != NULL) { *inError = NULL; } // Initialized to something
known so a bogus pointer isn't accidentally used.
host = [inHost copy];
port = inPort;
if(([self getAddress] && [self createSocket] && [self connect] &&
[self updateStream]) == NO) {
if(inError != nil) { *inError = [[error copy] autorelease]; }
return(nil);
}
return([self retain]);
}
- (void)dealloc
{
if(addressInfo != NULL) { freeaddrinfo(addressInfo); addressInfo = NULL; }
if(socketFD > 0 ) { close(socketFD); socketFD = 0; }
if(host != NULL) { [host release]; host = NULL; }
[super dealloc]
}
Well, hey, at least it's a second opinion, for what that's worth...
which is probably not a whole lot. :)
_______________________________________________
Cocoa-dev mailing list (email@hidden)
Please do not post admin requests or moderator comments to the list.
Contact the moderators at cocoa-dev-admins(at)lists.apple.com
Help/Unsubscribe/Update your Subscription:
This email sent to email@hidden