views:

105

answers:

3

I have seen a similar line of code floating about in Apples code:

(void)[[URLRequest alloc] initializeRequestWithValues:postBody url:verifySession httpHeader:nil delegate:self];

URLRequest is my own custom class. I didn't write this and I think the guy that did just grabbed it from Apple's example. To me this should leak and when I test it I'm pretty sure it leaks 16 bytes. Would it? I know how to fix it if it does but wasn't sure as it was taken from Apple's code.

EDIT: The problem was with the SDK, not the above code. See answer below for further details

+2  A: 

Not at all sure what this code is supposed to accomplish. It does appear to break every single convention about initialization methods. What's the point of returning a void pointer from an initialization method? The entire point of an initialization method is to return an object. Where in Apple's code examples did you see this?

Having said that, I don't see why it would leak. Since it doesn't return an object there is nothing to leak external to the method. There might be something internally that leaks.

Edit:

It basically does an NSURLConnection. Because we are submitting a lot of forms with a lot of different values we put it in an external class. All the delegate methods like didFailWithError: are in NSURLRequest and connectionDidFinishLoading just passes the data to its delegate. So it doesn't really need to return anything as it is done through a delegate method.

Yeah, you need to redesign this. At present, this method is just a disaster waiting to happening. If nothing else, everyone else looking at this code will be utterly confused about what you are doing.

If you have no need to retain the object created, then move its allocation and clean up entirely within a method. Change the method name prefix from "initialize" to something like "setup", "configure", "acquire" etc so the name doesn't imply that it creates and returns and object.

If you need a one shot instance of a particular class, use a class method like Michael Aaron Safyan suggested (again without initialize in the name.) The class method should internally initialize an instance, perform the operations needed, return the data to wherever, then dealloc the instance.

That way, you won't have to worry about leaks and everyone else who may read your code (including yourself months down the road) will immediately understand what the code does.

TechZen
It basically does an NSURLConnection. Because we are submitting a lot of forms with a lot of different values we put it in an external class. All the delegate methods like didFailWithError: are in NSURLRequest and connectionDidFinishLoading just passes the data to its delegate. So it doesn't really need to return anything as it is done through a delegate method.
Rudiger
Yeah. It took me ages to figure out and then the guy showed me Apple's demo code where he got it from. Think I might scrap the whole thing and start again.
Rudiger
+2  A: 

Yes. This is a leak, which can easily be fixed by adding an autorelease:

[[[URLRequest alloc] initializeRequestWithValues:postBody url:verifySession httpHeader:nil delegate:self] autorelease];

Perhaps a better fix would be to create a class function that does this:

@interface URLRequest
{
   // ...
}
// ...
+ (void) requestWithValues:/* ... */ 
// ...
@end

Then you could simply use [URLRequest requestWithValues: /* ... */] without invoking alloc.

Michael Aaron Safyan
I can't actually call autorelease because the NSURLConnection in URLRequest might respond after the run loop. I have tried it and it crashes. I'll look into your other fix though as it sounds better.
Rudiger
@Rudiger, if you need the instance to persist, then you should save it as a member of the object that is using it and use a normal release on the member in the dealloc method.
Michael Aaron Safyan
+1 A class method is definitely the way to go
TechZen
+4  A: 

Thought I might update this as after further testing and the release of iOS4 it has changed.

The above code doesn't leak and the memory footprint of the App returns to normal even after 200 iterations of the code. The leak did occur in iOS3 but was very small, in iOS4 it has completely disappeared both in simulator and device.

Some might wonder why you would want to implement this code but it works and make sense when dealing with lots of different NSURLConnections throughout your code running simultaneously.

Rudiger