views:

55

answers:

3

I have a controller that makes HTTP GET requests using a custom class, which acts as the delegate for the NSURLConnection. Once the NSURLConnection fails or finishes, the custom class invokes methods on the controller and passes along an NSData object of received data.

I'm running into a problem where the controller in action is being dynamically created and pushed onto a navigation controller's stack. This controller makes the HTTP GET request in its viewDidLoad method. If the user quickly presses "back" on the navigation bar, this controller gets dealloc'ed. If this happens before the HTTP GET request finishes, the resulting NSURLConnection callback becomes a method call to a dealloc'ed object, which results in an EXC_BAD_ACCESS.

What's the best approach to cleaning up any pending NSURLConnections that have been kicked off by a controller which may actually be deallocated already?

I threw in some NSLog statements, and it seems that my custom class used as a NSURLConnection delegate doesn't actually receive a dealloc message. I made sure to set the controller's instance of this class to nil in viewDidUnload, and also call release on it, but it still seems to live longer than the controller.

A: 

You need to retain your view controller when you make a request and release when the get request finished

YourViewController.m
- (void)callGetRequest {
   [self retain];
}

- (void)didFinishAllGetTask {
   [self release];
}
vodkhang
I have never seen this pattern in any Objective-C code before. It has a very bad code smell.
Shaggy Frog
I see a lot of people doing that, what's wrong here? I think it makes sense when you want to keep your viewcontroller around
vodkhang
Just because "a lot of people" do something doesn't make it a good idea. As for what's wrong, what you have here is a basic problem of an object overriding its own natural life cycle -- the thing that created the object should be in charge of how long it lives. Irrespective of all that, this approach is the wrong solution. The problem is when an object calls its delegate, but its delegate has been `dealloc`ed -- so the solution is to make sure the delegate can't be called, not to have a view controller sticking around (perhaps indefinitely).
Shaggy Frog
It is not a wrong solution if you think of it in another way : when the object call its delegate, this delegate can be dealloced, then we shouldn't let the delegate dealloced until all of its objets has finished.
vodkhang
In objective-C, you cannot in charge 100% of the life cycle of an object, you can own it, by declaring retain and stop owning it by calling release on that object. So, it makes sense for me that the object can own itself. I say a lot of people do it because your first argument was that "I have never seen this pattern"
vodkhang
To be honest, we already tried with your approach before, but in our case we have 3 or 4 layers of delegates (A->B->C->D). If your A owns B around, in dealloc, you have to release your B, right? And if you release B, you have to set C.delegate = nil and also release C. The problem finally comes when you touch the library code. You cannot just release D when D is the library object and you are not sure if D is delegate of something else?
vodkhang
Even in the simple case, who and when will release your someObject in someObject.delegate = nil; You cannot release it in your delegate dealloc method, because it is still working with the network
vodkhang
@vodkhang you completely misunderstand my solution. Please read it again carefully.
Shaggy Frog
A: 

If the user quickly presses "back" on the navigation bar, this controller gets dealloc'ed. If this happens before the HTTP GET request finishes, the resulting NSURLConnection callback becomes a method call to a dealloc'ed object, which results in an EXC_BAD_ACCESS.

When the user hits the back button, de-register the view controller class from being the delegate for that object. (Keep a reference to the object in the view controller class, so you can do something like someObject.delegate = nil;). You can do that in the view controller's dealloc method.

Shaggy Frog
If your class keeps someObject, Who and when will it release your someObject; You cannot release it in your delegate dealloc method, because it is still working with the network
vodkhang
@vodkhang Please read my answer again, as you didn't understand it. I didn't say to *release* the object. I said to *unset the delegate* of the object, which can be done in the *delegate's* `dealloc` method.
Shaggy Frog
I understand that you need to unset the delegate of the object. But my question is that how about the object itself. You create the object (A), call the object A to do networking job, and then who and when will release the object A? I think you have a memory leak here
vodkhang
If the view controller is creating the object, then it should destroy the object when it itself is `dealloc`ed.
Shaggy Frog
but then your object is still doing networking job, right? If you destroy the object in your dealloc, then you don't need to set someObject.delegate = nil, right? Look at the viewcontroller's dealloc method again. You have both lines `someObject.delegate = nil; [someObject release];`. Then why do you need the first line, anyway? It also that your someObject can be a delegate of somebody doing networking, and you are still crashed
vodkhang
A: 

If I understand it correctly you just need to do [whateverConnection cancel] in your viewDidUnload or dealloc method. This cancels the connection. It is almost the same if you have a custom downloader object for example for some large image that uses NSURLConnection. Make a cancel method for your class (that cancels the connection and releases it) and invoke it in your controller's dealloc method. You should also use a bool flag something like wasCanceled and do not invoke any method from your custom object's delegate if wasCanceled was set from your cancel method. (You only have a weak pointer to your delegate so it is probably released already when some other object invokes your cancel method). I assume that the delegate for your custom object is the view controller. I had several downloaders like this and it worked ok, without leaks even if I quickly canceled a download.

@interface CompaniesDownloader : NSObject /*<NSXMLParserDelegate>*/
{
    id<CompaniesDownloaderDelegate> delegate; //a view controller is the delegate
    NSMutableArray *companies;

    BOOL isWorking;    
    BOOL wasCanceled;   

    @private

    //url connection object
    NSURLConnection *companiesConnection;

    //this is where i put the binary data that gets transformed into xml
    NSMutableData *webData;

    //temporary string used when parsing xml
    NSMutableString *tmpString;

    //temporary company used when parsing xml
    Company *tmpCompany;
}

In the implementation:

-(void) cancel
{
  [[UIApplication sharedApplication] setNetworkActivityIndicatorVisible: FALSE];

  wasCanceled = TRUE;
  [companiesConnection cancel];
  [webData release];
  webData = nil;
  self.companiesConnection = nil; //OR [companiesConnection release]; companiesConnection=nil;
  isWorking = FALSE;
}
kudor gyozo
Cool, this sounds like the way to go. This is more or less what some of the Apple sample code does (see LazyTableLoading) when using custom downloader objects that are essentially just delegates for NSURLConnections.
pmc255
the only problem with this way is that if you call a library to do the networking job, then it is less likely that you can call cancel on it:(
vodkhang
Well for my networking needs this was enough. If it has some problems please let me know, I actually didn't look at the apple samples when I was making this.
kudor gyozo