Memory leak when setting a UIImage in an NSOperation

Question

I'm having an issue where relatively large images never seem to get released from memory (1MB~5MB in size). This following block of code gets called while a user scrolls through a set of images. After about 15 images the application will crash. Sometimes "didReceiveMemoryWarning" gets called, and sometimes it doesn't--the application will just crash, stop, quit debugging, and not halt on any line of code--nothing. I assume this is what happens when the device runs out of memory? Another concern is that 'dealloc' never seems to get called for the subclassed 'DownloadImageOperation'. Any ideas?

Getting and setting the image:

//Calling this block of code multiple times will eventually cause the
// application to crash

//Memory monitor shows real memory jumping 5MB to 20MB increments in instruments.  
//Allocations tool shows #living creeping up after this method is called.
//Leaks indicate something is leaking, but in the 1 to 5 kb increments. Nothing huge.

DownloadImageOperation * imageOp = [[DownloadImageOperation alloc] initWithURL:imageURL localPath:imageFilePath];
[imageOp setCompletionBlock:^(void){
    //Set the image in a UIImageView in the open UIViewController.
    [self.ivScrollView setImage:imageOp.image];
}];
//Add operation to ivar NSOperationQueue
[mainImageQueue addOperation:imageOp];
[imageOp release];

DownloadImageOperation Definition:

.h file

#import <Foundation/Foundation.h>

@interface DownloadImageOperation : NSOperation {
    UIImage * image;
    NSString * downloadURL;
    NSString * downloadFilename;
}

@property (retain) UIImage * image;
@property (copy) NSString * downloadURL;
@property (copy) NSString * downloadFilename;

- (id) initWithURL:(NSString *)url localPath:(NSString *)filename;

@end

.m file

#import "DownloadImageOperation.h"
#import "GetImage.h"

@implementation DownloadImageOperation

@synthesize image;
@synthesize downloadURL;
@synthesize downloadFilename;

- (id) initWithURL:(NSString *)url localPath:(NSString *)filename {

    self = [super init];

    if (self!= nil) {
        [self setDownloadURL:url];
        [self setDownloadFilename:filename];
        [self setQueuePriority:NSOperationQueuePriorityHigh];
    }

    return self;

}

- (void)dealloc { //This never seems to get called?
    [downloadURL release], downloadURL = nil;
    [downloadFilename release], downloadFilename = nil;
    [image release], image = nil;
    [super dealloc];
}

-(void)main{

    if (self.isCancelled) {
        return;
    }

    UIImage * imageProperty = [[GetImage imageWithContentsOfFile:downloadFilename andURL:downloadURL] retain];
    [self setImage:imageProperty];
    [imageProperty release];
    imageProperty = nil;
}

@end

Get Image Class

.m file

+ (UIImage *)imageWithContentsOfFile:(NSString *)path andURL:(NSString*)urlString foundFile:(BOOL*)fileFound {

    BOOL boolRef;

    UIImage *image = nil;

    NSString* bundlePath = [[NSBundle mainBundle] bundlePath];

    if (image==nil) {
        boolRef = YES;
        image = [UIImage imageWithContentsOfFile:[[AppDelegate applicationImagesDirectory] stringByAppendingPathComponent:[path lastPathComponent]]];
    }
    if (image==nil) {
        boolRef = YES;
        image = [super imageWithContentsOfFile:path];
    }
    if (image==nil) {
        //Download image from the Internet
        [[UIApplication sharedApplication] setNetworkActivityIndicatorVisible:YES];

        NSURL *url = [NSURL URLWithString:[urlString stringByAddingPercentEscapesUsingEncoding:NSUTF8StringEncoding]];

        ASIHTTPRequest *request = [ASIHTTPRequest requestWithURL:url];
        [request setTimeOutSeconds:120];
        [request startSynchronous];

        NSData *responseData = [[request responseData] retain];

        [[UIApplication sharedApplication] setNetworkActivityIndicatorVisible:NO];

        NSData *rdat = [[NSData alloc] initWithData:responseData];
        [responseData release];

        NSError *imageDirError = nil;
        NSArray *existing_images = [[NSFileManager defaultManager] contentsOfDirectoryAtPath:[path stringByDeletingLastPathComponent] error:&imageDirError];

        if (existing_images == nil || [existing_images count] == 0) {
            // create the image directory
            [[NSFileManager defaultManager] createDirectoryAtPath:[path stringByDeletingLastPathComponent] withIntermediateDirectories:NO attributes:nil error:nil];
        }

        BOOL write_success = NO;
        write_success = [rdat writeToFile:path atomically:YES];

        if (write_success==NO) {
            NSLog(@"Error writing file: %@",[path lastPathComponent]);
        }

        image = [UIImage imageWithData:rdat];
        [rdat release];

    }

    return image;
}

Apologies for this huge block of code. I really have no idea where the problem might be here so I tried to be as inclusive as possible. Thanks for reading.

Answer 1

The main issue in the operation not getting deallocated is that you have a retain cycle, caused by the reference of imageOp in the completion block. Consider your code that says:

DownloadImageOperation * imageOp = [[DownloadImageOperation alloc] initWithURL:imageURL localPath:imageFilePath];
[imageOp setCompletionBlock:^(void){
    //Set the image in a UIImageView in the open UIViewController.
    [self.ivScrollView setImage:imageOp.image];
}];

In ARC, you would add a __weak qualifier to the operation and use that rather than imageOp within the completionBlock, to avoid the strong reference cycle. In manual reference counting, you can avoid the retain cycle through the use the __block qualifier to achieve the same thing, namely to keep the block from retaining imageOp:

DownloadImageOperation * imageOp = [[DownloadImageOperation alloc] initWithURL:imageURL localPath:filename];
__block DownloadImageOperation *blockImageOp = imageOp;
[imageOp setCompletionBlock:^(void){
    //Set the image in a UIImageView in the open UIViewController.
    [self.imageView setImage:blockImageOp.image];
}];

I think if you do that, you'll see your operation getting released correctly. (See the "Use Lifetime Qualifiers to Avoid Strong Reference Cycles" section of Transitioning to ARC Release Notes. I know you're not using ARC, but this section describes both the ARC and manual reference counting solutions.)

---

If you don't mind, I had other observations regarding your code:

• You shouldn't be updating the UI from the completionBlock without dispatching it to the main queue ... all UI updates should happen on the main queue:

  DownloadImageOperation * imageOp = [[DownloadImageOperation alloc] initWithURL:imageURL localPath:filename];
  __block DownloadImageOperation *blockImageOp = imageOp;
  [imageOp setCompletionBlock:^(void){
      //Set the image in a UIImageView in the open UIViewController.
      UIImage *image = blockImageOp.image;
      [[NSOperationQueue mainQueue] addOperationWithBlock:^{
          [self.imageView setImage:image];
      }];
  }];
  

• You're using accessor methods in your init method. As a matter of good practice, you really shouldn't. See Don’t Use Accessor Methods in Initializer Methods and dealloc in the Advanced Memory Management Programming Guide.

• While we may have fixed the problem of the operation not getting released, I would suspect that unless you've already coded your UIScrollViewDelegate calls to release images that have scrolled off the visible screen, that you'll continue to have memory issues. Having said that, you may have already tackled this issue, and if so, I apologize for even mentioning it. I only raise the issue as it would just be easy to fix this NSOperation problem, but then neglect to have the scroll view release images as they scroll off the screen.

• I'm not sure your subclassed NSOperation will support concurrency as you're missing some of the key methods discussed in Defining a Custom Operation in the Concurrency Programming Guide. Perhaps you have done this already, but just omitted it for brevity. Alternatively, I think it's easier if you use one of the existing NSOperation classes (e.g. NSBlockOperation) which take care of this stuff for you. Your call, but if you pursue concurrency, you'll want to make sure you set the queue's maxConcurrentOperationCount to something reasonable, like 4.

• You code has some redundant retain statements. Having said that, you have the necessary release statements, too, so you've ensured that you won't have problems, but it's just a little curious. Clearly, ARC gets you out of the weeds on that sort of stuff, but I appreciate that this is a big step. But when you get a chance, take a look at ARC as it saves you from having to worry about a lot of this.

• You should probably run your code through the static analyzer ("Analyze" on the "Product" menu), as you have some dead stores and the like.