views:

1179

answers:

7

I know the basic principles about memory management (retain count, autorelease pools etc) in Cocoa, but once you go past simple retain/release, it's getting a bit more confusing. I couldn't find decent answers for those, as most tutorials cover simple scenarios. I would like to ask about best practices in how to write the code and avoid leaks.


1st question - iterations and temporary assigments:

for (id object in objectArray) {    
     Model *currentItem = object;
    /* do something with currentItem */
    [currentItem release];
}

If I remove the release in last line the code will work fine, but there's leak. What's the rule here? The object is already there in objectArray. I'm assigning it directly, to get the type. Should I do this some other way? Is this assignment increasing retainCount of currentItem? (is it something like [[alloc] initWithObject] ?) How to know if this assignment (object) is autoreleased or not?


2nd question - instant retains :

Model *model = [unarchiver decodeObjectForKey:@"ARCHIVED_MODEL_OBJECT"];
// it has to be here, because (I was told) unarchiver will return autorelease object    
[model retain]; 
label.text = model.data;

How someone knew that this particular method works so weird, that I need to instantly call retain on returned value, or I will bump into null on next assignment? I couldn't find anything like this in documentation. According to retain/release rules, I would expect that decodeObjectForKey returns autorelased object, but it will take some time until the control goes back to app and pool claims the model object to be released. Is there any rule for that? How should I search for those?


3rd question - autoreleasing and passing variables:

- (IBAction) loadXMLButtonClicked:(id) sender {
    objectArray = [self loadData]; // 1 - objectArray is instance var
    NSArray *objectArray = [self loadData]; // 2 - objectArray is local var

    // loadXMLButtonClicked is called on button click, here the method finishes
    // and control goes back to application, autorelease pool is cleaned?

    // case 1 - objectArray stays retained in instance variable? (because setter was used)
    // case 2 - objectArray is soon to be released, there were no retains? 
    // (ignore the fact that it's local var, just hypothetically)

}

- (NSArray *) loadData {
    NSArray *objectArray = [[NSArray alloc] init];
    // populate array here
    return [objectArray autorelease];
}


4th question - (bear with me, last one) parser xml best practice : (i don't want to use other solutions, using standard parser is to practive objective-c memory management and flow)

Basically this code here works, works good and have no leaks, but I don't really know if this is proper approach. I have separate object that acts as parser, parses an XML to collect an array of objects of type Model. Now, after parsing done, externally I would like to obtain that array, though I don't know how (is copying the array and releasing whole parser good idea?). Please run through the code and see the comments.

I've debug this code many times, using gdb for printing retainCounts, zombies, etc. and while I can manage to get this running and without leaks, I don't know 100% why and would like to hear a good reasoning how should this be done with explanation. That would be much appreciated.

Controller.m

- (NSArray *) loadData {
(...)
NSXMLParser *parser = [[NSXMLParser alloc] initWithData:data];
ModelXMLParser *parserDelegate = [[ModelXMLParser alloc] init];
[parser setDelegate:parserDelegate];
[parser parse];

objectArray = [[parserDelegate objectArray] copy]; 
// is this ok? i don't need the parser object so I think i should get rid of it
// and copy the data. how this copy works, is it shallow (only new reference to array)
// or deep copy (objects allocated again as well)?
// how to do deep copy of NSArray?

[parserDelegate release];
[parser release];
}

ModelXMLParser.m (simplified)

@implementation ModelXMLParser

@synthesize objectArray; // array of objects
@synthesize currentObject; // temporary object
@synthesize currentChars; // temporary chars
@synthesize parseChars; // parse chars only when there's need, leave those /t/n etc

- parser didStartElement (...) {
    if ([elementName isEqualToString:@"objects"]) {
        objectArray = [[NSMutableArray alloc] init];
    }
    else if ([elementName isEqualToString:@"object"]) {
        currentObject = [[Model alloc] init];
    }
    else if ([elementName isEqualToString:@"name"]) {
        // do i have to init currentObject.name (NSString) here? i guess not?
        [self setParseChars:YES]; // just set the flag to make parse control easier
    }
    else if ([elementName isEqualToString:@"number"]) {
        // int isn't object anyway, no init
        [self setParseChars:YES]; // just set the flag to make parse control easier
    }   
}

- parser foundCharacters (...) {
    if (parseChars) {
        currentChars = [[NSString alloc] initWithString:string];
        // why is currentChars retainCount = 2 here?
        // is it like currentChars = [NSString new] and then currentChars = string? (so retain once more)
        // is it good way to control parser? (please ignore the NSMutableString and appending example, try this one)
        // should I just do currentChars = string here?

        [currentChars autorelease]; // this is currently my solution, because there's no leak, but i feel it's incorrect?
    }
}

-  parser didEndElement (...) {
    if ([elementName isEqualToString:@"object"]) {
        [objectArray addObject:[currentObject copy]]; // should I copy here or just addObject, it retains anyway?
        [currentObject release]; // I've initialized currentObject before, now I don't need it, so i guess retainCount goes to 0 here?
    }
    else if ([elementName isEqualToString:@"name"]) {
        currentObject.name = currentChars; // is this correct, or shoud I do [currentChars copy] as well?
        [self setParseChars:NO];
        [currentChars release]; // as before, initialized, now releasing, but is this really correct?
    }
    else if ([elementName isEqualToString:@"number"]) {
        currentObject.number = [currentChars intValue]; // is this correct, or shoud I do [currentChars copy] as well?
        [self setParseChars:NO];
        [currentChars release]; // as before, initialized, now releasing, but is this really correct?
    }   
}

- (void) dealloc {
    // i shouldn't release currentChars or currentObject, those (i suppose) should be freed after parsing done,
    // as a result of earlier releases?

    [objectArray release];
    [super dealloc];
}

Model.m

@implementation Model

@synthesize name; // this is NSString
@synthesize number; // this is int

- (id) copyWithZone:(NSZone *) zone {
    Model *copy = [[[self class] allocWithZone:zone] init];
    copy.name = [self.name copy];
    copy.number = self.number;
    return copy;
}

- (void) dealloc {
    [name release];
    // i dont have to release int, right? it's not an object
    [super dealloc];
}

I'm especially confused with question 4. Sorry for maybe too long question, but this really is about one topic and deeper understanding of it.

+1  A: 

@1st question

You only get a reference to the object in the objectArray. It is still in the objectArray which also has the object retained and releasing it is not good here because you didn't do anything that retained it.

See here for some rules

epatel
Makes sense to me. But if I don't do this, I have memory leak. The object array is autoreleased object, so I shouldn't release it. Why the leak?
rattkin
How do you know you have a leak? Epatel is right, you must not release the object. Aren’t you by a chance doing something like [objectArray addObject:[[foo alloc] init]? That would be the real cause of the leak.
zoul
A: 

@2nd question

Looks like you are setting a UILabel text property, which in this case uses copy. In the documentation it says:

@property(nonatomic, copy) NSString *text;

This means the Label will copy and retain that copy not changing or retaining the object used to assign the property.

epatel
I know I can do [model release] after. The question is why do I have to retain the model after this unarchiver method? If I don't do this, the model will be freed, and assigment to label will fail.
rattkin
A: 

@3rd question

The line "objectArray = [self loadData]; // 1 - objectArray is instance var" isn't really a setter as it accesses the instance variable directly. To use a setter one needs to access it though self

self.objectArray = [self loadData];

...and if your property is declared as (nonatomic, copy) the old objectArray will be released and a new will be created with a copy, thus being retained.

epatel
A: 

I think a good read of Apple's Cocoa Memory Management Rules is in order. Many of your questions are answered expertly in that document.

August
+7  A: 

1st question - iterations and temporary assigments

You should NOT be releasing the objects here since you are not an owner. You should only ever release an object if you are an owner of it. See the Memory Management Guide for Cocoa. You are only an owner of an object if you call a method whose name begins with init, new, or contains copy in its name.

Since the for loop does not use methods with any of those names, you are not an owner, so you MUST NOT release the objects. This will result in freeing objects before they're done, which will almost certainly result in memory corruption and a crash.

2nd question - instant retains:

You don't need to instantly call retain, you just need to call it before the autorelease pool next empties. This will probably be shortly after your method returns to the main event loop. Since you don't know exactly when this will happen, you have to make sure that if you want to be able to access the object after the function (loadXMLButtonClicked: in this case) returns, then you must retain it before you return.

Since decodeObjectForKey does not begin with init or new or contain copy in its name, you are not becoming an owner. Calling retain makes you an owner.

3rd question - autoreleasing and passing variables:

First of all, it's bad practice to shadow a class member with a local variable of the same name. Secondly, unless loadData is being used as a multi-purpose utility function (which I'm guessing it isn't since it doesn't take any parameters), it should just assign the result directly to the member variable objectArray. It's pointless and error-prone to return the result and then have the calling function assign to the member variable.

Thirdly, you're not using the objectArray property setter -- you're just assigning straight to the member variable. If you want to use the setter, you have to explicitly say self.objectArray = ... (with the self. in front of it). Thus, objectArray is never retained, so it's going to be deallocated the next time the autorelease pool clears itself out, which is not what you want. You must retain it at some point, or conversely, just don't autorelease it at the end of loadData, and assign it the class member variable objectArray.

If the property is declared with the retain attribute, then using the setter will automatically call retain when you assign with it (and it will also release the old value). If instead the property is declared with the copy attribute, then the value will instead be copied every time you assign to it, and you will become an owner of the new object.

4th question - parser xml best practice:

You're making a shallow copy of the object array. If you want to make a deep copy, you can use the initWithArray:copyItems: message.

do i have to init currentObject.name (NSString) here? i guess not?

I don't understand this question, there's no currentObject.name mentioned at all anywhere nearby that code.

why is currentChars retainCount = 2 here?

Probably because during its internal initialization process, it was retained an extra time somewhere, but it was also almost certainly autoreleased an extra time as well. If you follow all of the rules from the Memory Management Guide for Cocoa, you won't have any problems. You should never rely on retain counts, since you don't know how many times an object has been autoreleased. They're a debugging aid, not something that should be used for program control flow.

this is currently my solution, because there's no leak, but i feel it's incorrect?

If you won't need to use currentChars the next time you return to the event loop, then it's fine. If you will need to use it, you should not release or autorelease it here, and then release it when you're sure you're done with it.

should I copy here or just addObject, it retains anyway?

Just addObject: when you add items into an NSArray, NSSet, or NSDictionary, they are automatically retained by the data structure. When you remove them, they're released.

Most of the rest of the questions can be answered by just following the rules or have identical answers to some of the previous questions which I've already answered.

Adam Rosenfield
A: 

Thanks to all for answers. Those answers helped, though at few places only, the rest I had to figure out myself. I was much more focusing on the idea, not line-by-line implementation, but it was hard to describe here without pasting huge chunks of code. It's just that delegate for xml object parsing is a very specific example, because it doesn't return value per se, the value has to be taken and assigned externally.

I'm marking Adam's answer as best one, as being most-detailed, even though not answering all my problems.

For others - http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/MemoryMgmt.html is an excellent read. Also read my own answers to questions :

1 - Of course I shouldn't release that object. The memory leak wasn't caused by this.

2 - This method is nothing special, it just returns plain autoreleased object, as others tried to explain to me here. The root of my initial troubles was that I previously didn't have that retain, but called [model release] soon after, causing autorelease pool sending release to non-existant object. I shouldn't do [model release] because I'm not the owner of that object. In fact, this retain here isn't even needed, because I just need the object to get value from it, and then I can thrash it, so it can be safely passed to autorelease pool, without retain.

3 - I intended this method (loadData) to be independant, therefore not setting any instance variables, but returning array for others. It was a parallel example, it's not that I have two variables with the same name in method.

If I declare object within this method (situation #2), then just so it happens that it's autoreleased at the end of this method, because after it finishes, the control goes back to application and release pool kicks in. It was ok with me in this example, because I don't need the array later on. In real world I should probably have instance variable (situation #1) and then go with self.objectArray = [self loadData], because this will launch setter and the autoreleased object will be retained for me here.

4 - I've confused few things here. Basically I was trying to code in objecive-c with manual memory management, but still having "garbage collector" attitude. It's very important to remember that if you do [[object alloc] init] and then at later time [object release] - it doesn't automatically have to be that the object will be destroyed! Release isn't nullifying! This is of course a fundamental rule (retain/release) but even knowing it, it's easy to forget. Track what you are doing with your object between those two lines - the object may actually live for a long long time after, because "someone" else will become its owner. The release method at the end of this class livecycle doesn't mean - object is now destroyed, but means : "I no longer care, my hands are clean"

So, line by line :

objectArray = [[parserDelegate objectArray] copy];

This is perfectly fine, I don't deep copy. I'm copying an array, which means it allocates new memory for the array object, but not for the contents. BUT, copy sent to objectArray also sends retain to each object. In my example, I'm releasing my parserDelegate which also releases its own objectArray, decreasing retainCount for each object. If I wouldn't do the copy here, objects will reach retainCount = 0 and be destroyed. This way I have new array, with pointers to old objects, but they essentially become my objects, because previous array is destroyed, and because of my retain, I'm becoming the owner. Sorry if this is talking a bit too much, but I really had to focus to understand this.

else if ([elementName isEqualToString:@"name"]) {
    // do i have to init currentObject.name (NSString) here? i guess not?
    [self setParseChars:YES]; // just set the flag to make parse control easier
}

The question here was to, whether I should initialize currentObject.name NSString property, because it will be filled soon after foundCharacters kicks in. Now this is interesting. When you initialize whole object, its NSString properties are nil. Now, later, I do

currentObject.name = currentChars;

Which launches setter. This setter is defined as (nonatomic, retain), which means that new value is retained, old value is released and pointer is assigned. Funny enough, it doesn't matter if the previous value is initialized or is nil - if it is initialized it will be released anyway, if it is nil, then nil can still accept release (though I'm not 100% sure of it?) - nothing will happen. But for the sake of correctness, I guess the initial line should look like :

else if ([elementName isEqualToString:@"name"]) {
    currentObject.name = [NSString new]; // just alloc/init routine
    [self setParseChars:YES]; // just set the flag to make parse control easier
}

Now :

[currentChars autorelease];

Should not be there. This is confusing design and it was a bad fix. All it should be there, is just string init.

[objectArray addObject:[currentObject copy]];

Copy is unnecessary here. addObject will retain anyway. No point in creating another allocation. This was my one of causes of my leaks.

Everything else is fine. Because I'm releasing currentChars right after setting the value to my object, he now retains it, and becomes the owner, and I'm just releasing it "here" in parser, because I don't need it anymore (It will be allocated in next loop).

It might be confusing, but I can imagine, there will be others with non-standard memory assignment problems and even for experienced people it might be a challenge to put things in right places. Maybe my story will help then.

rattkin
A: 

Static Analyzer

In addition to: Memory Management Programming Guide for Cocoa , Static Analyzer is an indispensable tool.

Project->Project Settings->Build->Build Options->Run Static Analyzer

Make sure that it is ticked.

It will tell you all memory allocation errors you are doing. So, you will understand better how to create objects, double-autorelease errors, double-release errors, referring to released objects, etc.

I read the memory management principles many times but did not get it until I used the Static Analyzer.

Now I am better at this and get it right most of the time. Static Analyzer, however, is still essential because it points out mistakes and missed ones.

Yoichi

Yoichi