0

Is it OK to retain a delegate of a subclass of ASIHTTPRequest?

I made a subclass of ASIHTTPRequest called JSONRequest. Each instance of JSONRequest is its own delegate, handles the callbacks, and passes them on to jsonDelegate, which is a private property of JSONRequest, and responds to requestFinished:withResult:, where result is an NSDictionary representation of the JSON response. To do this, I overloaded setDelegate: in JSONRequest to do super.delegate = self; self.jsonDelegate = newDelegate.

Is it OK to retain jsonDelegate in this case because often jsonDelegate is a view controller, which is sometimes dealloced while the request is loading if the user hits "Back," etc. I will release jsonDelegate in JSONRequest after calling the callback methods.

How do I know this is OK and will not cause a retain loop.

Community
  • 1
  • 1
ma11hew28
  • 121,420
  • 116
  • 450
  • 651

3 Answers3

5

What's retaining the request? (The operation queue, perhaps? Who knows?)

In general, the "fire-and-forget-and-give-me-a-callback" method you seem to be proposing is a bad idea. If nothing is retaining the VC apart from the request, then (unless your app structure is a bit silly) the VC will never get to do anything with the data it receives, so there's no reason for it to continue.

It also feels wrong: Does the request own the VC, or does the VC own the request? I'd expect the latter, so the VC should also retain the request.

There are a couple of exceptions:

  • CAAnimation.delegate is retained, presumably because the animation is going to complete at some point (I'm not sure what happens if it's a repeating animation). The same may be true of UIKit's animation delegate
  • NSTimer retains its target, probably because NSInvocation does. (I wrote a "weak timer" class to work around this.)
  • CADisplayLink retains its target, presumably to be like NSTimer.

For these cases, I often work around it with a "weak proxy" class which doesn't retain its target (and I wrote a "weak timer" wrappers around NSTimer/CADisplayLink to make this a bit easier.)

What you're supposed to do is keep track of the requests that you've initiated and in dealloc, do something like

request.delegate = nil;
[request cancel];
self.request = nil;

Similarly, you're supposed to unregister for notifications/actions/KVO callbacks at the appropriate time. There's an exception:

  • If you're sure that it won't send you a callback after you release it, you don't need to bother, so text field delegates and button action/targets don't need to be cleared.

There are exceptions to the exception:

  • UIWebView (at least in older OSes) are also retained by something else, possibly something to do with the web thread. It can crash if the VC disappears while the web view is still loading.
  • UIScrollView scrolling callbacks also cause the view to be retained past the lifetime of the VC. You can test this by e.g. holding "Done" and starting a flick as you release "Done".
tc.
  • 33,468
  • 5
  • 78
  • 96
  • +1, agreed. Better answer than mine... but I was just answering the question directly... this is a better overarching answer. – Steve Jun 30 '11 at 01:55
  • @tc, thank you for the thorough answer. I do agree with much of it. However, for networking, I believe the connection should retain its delegate just like [`NSURLConnection` retains its delegate](http://stackoverflow.com/questions/1101867/does-a-nsurlconnection-retain-its-delegate/6529335#6529335). For this reason and many others, I much prefer `NSURLConnection` to `ASIHTTPRequest`, but switching to `NSURLConnection` will take some time. – ma11hew28 Jun 30 '11 at 03:07
  • The reason retaining the delegate is nice here is that if the nav controller (NC) pops the view controller (delegate) while the request is still loading, the VC may be dealloced, causing an `EXEC_BAD_ACCESS` exception. But, in this case, I want the delegate to be retained until after the request finishes loading so that the delegate can handle success or errors. – ma11hew28 Jun 30 '11 at 03:10
  • Here's what I'll make sure to do though: I will not fire & forget. Instead, I will have the request retain the delegate and release it when the request completes. That way, if the nav controller (NC) pops & releases the VC during a request, the VC will still be able handle the completion of the request via delegate callbacks. I will just handle the callbacks differently based on whether the [UIViewController is visible](http://stackoverflow.com/questions/2777438/how-to-tell-if-uiviewcontrollers-view-is-visible/2777460#2777460). – ma11hew28 Jun 30 '11 at 03:22
  • I don't think VCs should generally be responsible for handling the result of a network request. There are various ways around this, a simple one being a "network controller" instead. – tc. Jul 01 '11 at 02:43
  • @tc, OK, but I want the VC to update its view (if it's visible, or perhaps just loaded) based on the network response. How would I do that from the network controller, with a notification? Would the network controller handle all requests for one app and just tag each type or request or something to distinguish between them? – ma11hew28 Jul 02 '11 at 12:57
  • That depends on the type of request, the underlying data model, and whether important thing is the request ("mail sent") or the change to the model ("outbox is empty"). In the former case, you might want to associate the VC with the request; in the latter case, the change to the model (notified via Core Data or a NSNotification) seems to make sense. The difference is whether it's possible to have a different VC viewing the "same" data, and whether you want to show progress in both of them. – tc. Jul 24 '11 at 21:36
2

If your release is deterministic (if you promise it will always happen, even in the case of an error, a timeout, or some other sort of unexpected event - like the connection simply hanging indefinitely) - then this is fine (no retain loop).

In this case, the retain loop would always get broken at some point.

Steve
  • 31,144
  • 19
  • 99
  • 122
  • Only if you un-set the delegate when the request finishes. – tc. Jun 30 '11 at 01:37
  • @tc: from the post: "I will release jsonDelegate in JSONRequest after calling the callback methods." – Steve Jun 30 '11 at 01:38
  • And then, the server can still return one byte every few seconds... It may be convenient, but it's bad practice. Retain what you own. – tc. Jun 30 '11 at 02:52
  • "unexpected events" are awful hard to expect. – Steve Jun 30 '11 at 03:04
  • @tc, why is it bad practice? [NSURLConnection retains its delegate until the request is complete](http://stackoverflow.com/questions/1101867/does-a-nsurlconnection-retain-its-delegate/6529335#6529335). – ma11hew28 Jul 02 '11 at 13:01
  • According to the [Cocoa Fundamentals Guide](http://developer.apple.com/library/ios/#documentation/Cocoa/Conceptual/CocoaFundamentals/CommunicatingWithObjects/CommunicateWithObjects.html), "Delegating objects do not (and should not) retain their delegates." Perhaps it does this for convenience (if you just want to "fire and forget" but run some code on completion), but it smells like bad design, and I've written silly amounts of method-forwarding code to avoid such retain cycles. – tc. Jul 24 '11 at 21:53
1

I know that this question is originally about ASIHTTPRequest, but people might stumble upon this thread and it might make them think that using ASIHTTPRequest is still best practice - it is not, infact development has stopped in 2011 AFAIK.

Using more modern HTTP libraries which use blocks by default (I prefer AFNetworking), all variables referenced in fail/success blocks are either copied or retained until the blocks are released. This will spare you this whole memory management headache - except if you use self in one of the blocks, then you will create a retain cycle until the blocks have been released.

manmal
  • 3,900
  • 2
  • 30
  • 42