On Sat, Dec 6, 2008 at 4:07 PM, Ken Tozier <[EMAIL PROTECTED]> 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 protected]) 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: http://lists.apple.com/mailman/options/cocoa-dev/archive%40mail-archive.com This email sent to [EMAIL PROTECTED]
