Handling memory leaks in factory methods

Question

I am developing an objective C framework which will ship as a static library at the end. But when I integrate that library to an actual application (by adding the static library) in the leaks tools I see some memory leaks present.

Here is an example scenario.

@implementation Test

@synthesize testNumber

+(Test) createTestInstance {

    Test *test = [[Test alloc] init];
    test.testNumber = [[NSDecimerNumber alloc] initWithInt:1];

    return test;
}

-(void) dealloc {
    [testNumber release];
}

@end

Although I release testNumber variable in dealloc I see a memory leak in Leaks tool at alloc position. What can be the issue here?

Also as this is a library provided for user to invoke, is it a best practice to release those variable from the library code?

Thank You

this is a singleton right? It's missing some things. On stackoverflow is a question answered with the best singletons. Just search a bit! — cocos2dbeginner, Jun 16 '11 at 12:44
I see nothing in the question or code that indicates a singleton. — Caleb, Jun 16 '11 at 13:02
This is not a singleton. I know there are lot of issues. The best way to do is to make it a instance variable. Just wanted to check the memory behavior in this situation. — Dilshan, Jun 16 '11 at 13:29

albertamg · Accepted Answer · 2011-07-11T17:47:18.050

11

I see two problems here. If testNumber is a retain property, you are overretaining it with this statement:

test.testNumber = [[NSDecimerNumber alloc] initWithInt:1];

Both alloc-init and the property accessor are retaining the object. Therefore, it should be:

test.testNumber = [[[NSDecimerNumber alloc] initWithInt:1] autorelease];

There is no need to mention that you still need to release testNumber in the dealloc method.

Also, I understand createTestInstance is a convenience constructor to create Testobjects and it should return an autoreleased object according to the Object Ownership Policy (only methods with names that start with “alloc”, “new”, “copy”, or “mutableCopy” return an object you own):

+ (id)createTestInstance {

    Test *test = [[[self alloc] init] autorelease];
    test.testNumber = [[[NSDecimerNumber alloc] initWithInt:1] autorelease];

    return test;
}

Finally, as suggested by @Josh Caswell, convenience constructors should return id instead of the specific class. From The Objective-C Programming Language:

The return type of convenience constructors is id for the same reason it is id for initializer methods, as discussed in “Constraints and Conventions.”

Also, they should use self instead of the hard-coded class name to alloc-init the instance in order to handle subclassing properly (self here refers to the class object itself since this is a class method).

edited Jul 11 '11 at 17:47

answered Jun 16 '11 at 12:43

albertamg

28,492
6
64
71

3

+1 You beat me to it. Ideally, you would also not add `create` to the method name as it sends a different message in terms of memory management. – Deepak Danduprolu Jun 16 '11 at 12:46
That means for init and alloc does it increment the retain count by 2? – Dilshan Jun 16 '11 at 13:32
1

Use `[NSDecimalNumber numberWithInt: 1]` to create the object assigned to `test.testNumber` – JeremyP Jun 16 '11 at 13:46
JeremyP yes thats fine. It will work. However this will autorelease if I do so. Like I mentioned this is a library. So the user should be able to access this property later after retrieval. – Dilshan Jun 16 '11 at 13:51
The user will be able to do so, because assigning to the property will retain it. That's what albertamg is getting at above - if you *don't* autorelease, you're creating a leak because the NSDecimalNumber instance is over-retained. – Sherm Pendley Jun 16 '11 at 14:12
1

@Deepak - You're referring to the "create rule," but this is Cocoa, not Carbon. The word "create" has no special meaning here with respect to memory management. – Sherm Pendley Jun 16 '11 at 14:13
Thanks Sherem. That means even though the NSDecimalNumber is auto released and as it is retained by testNumber property it will not crash even though the client application tries to consume it? – Dilshan Jun 16 '11 at 15:31
2

Constructor methods should return `id`, and use `self` instead of the hard-coded class name, or subclassing will be a pain. Consider what would happen to `NSMutableArray` if `+[NSArray array]` was `+ (NSArray *) array { return [[[NSArray alloc] init] autorelease]; }` (You're also missing the asterisk in the return value: `(Test)` should be `(Test *)`). – jscs Jun 16 '11 at 18:21
@Josh Caswell I updated the code to reflect your comments. Thank you for helping me improve my answer! – albertamg Jun 16 '11 at 19:24
Thank you all for the replies. One more question. How effective to use the factory method like this? Will it cause some synchronization issues here? – Dilshan Jun 17 '11 at 06:57

Handling memory leaks in factory methods

1 Answers1