NSOperation Subclass performance or leak

Question

Following is the subclass implementation of NSOperation Subclass The operation will be use to asynchronously download Image from server.

-(void) main{

    @autoreleasepool {
      //NSURLConnection
        NSURLRequest *request=[NSURLRequest requestWithURL:[NSURL URLWithString:@"A URl"]];
        _downloadConnection=[[NSURLConnection alloc] initWithRequest:request delegate:self];
    }
}

-(BOOL)isConcurrent{
    return YES;
}

-(BOOL)isExecuting{
    return _isExecuting;
}

-(BOOL)isFinished{
    return _isFinished;
}

-(void)finish{

    [self willChangeValueForKey:@"isExecuting"];
    [self willChangeValueForKey:@"isFinished"];

    _isExecuting = NO;
    _isFinished  =YES;

    [self willChangeValueForKey:@"isExecuting"];
    [self willChangeValueForKey:@"isFinished"];
}

-(void)cancel{

    [self willChangeValueForKey:@"isExecuting"];
    [self willChangeValueForKey:@"isFinished"];

    _isExecuting = NO;
    _isFinished  =YES;

    [self willChangeValueForKey:@"isExecuting"];
    [self willChangeValueForKey:@"isFinished"];
}


-(void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)error{
    [connection cancel];
    connection=nil;
    [self finish];
}

-(void)connectionDidFinishLoading:(NSURLConnection *)connection{
    //Other Code
    [connection cancel];
    connection=nil;
    [self finish];
}

Please let me know if there might be anything that I might have missed out within the code, so as to avoid leaks and check all KVO are handled properly.

Answer 1

I see a couple of issues:

1. Your finish and cancel routines are calling willChangeValueForKey twice for each key. Obviously that second call should be didChangeValueForKey.

2. I would advise against implementing cancel method. The default implementation does some other stuff. Do not implement that method. If you really want to, there are changes I'd advise here (at very least, cancel connection, too; call super; some other things, too), but I'd simply advise against implementing it at all and detect cancellation in didReceiveData (see point #5).

3. This code does not appear to be setting _isExecuting (nor do appropriate KVO) when the operation starts. Maybe that's in a start method you neglected to share with us?

4. Likewise, that start method should check to see if the operation has been canceled already and if so, stop the operation immediately.

5. In didReceiveData, are you checking isCancelled, as well? Making operations cancelable is one of the main reasons you use operation queues.

6. You're starting a NSURLConnection within an operation (presumably with the intent to add this operation to some random queue). But NSURLConnection will not work correctly unless you either schedule it on the main run loop (the easy solution) or you create your own run loop alive (there are a variety of techniques of that).

   For example, to tell the operation to schedule the connection in the main run loop, you could do something like:

   _downloadConnection = [[NSURLConnection alloc] initWithRequest:request delegate:self startImmediately:FALSE];
   [_downloadConnection scheduleInRunLoop:[NSRunLoop mainRunLoop] runLoopModes:NSRunLoopCommonModes];
   [_downloadConnection start];
   

   I'm typing that without benefit of Xcode, so if I mistyped a method forgive me, but it illustrates the idea: Create connection with startImmediately of FALSE, schedule it on the main run loop, and only then should you start it.

7. If connectionDidFinishLoading is called, it is entirely unnecessary to call [connection cancel].

8. As of iOS 7, isConcurrent has been deprecated in favor of isAsynchronous. If you need to support earlier iOS versions, though, keep isConcurrent.

9. By the way, while I think it might work the way you've laid it out, it is generally advised to implement properties called executing and finished:

    @property (nonatomic, readwrite, getter=isExecuting) BOOL executing;
    @property (nonatomic, readwrite, getter=isFinished)  BOOL finished;
   

   I then manually synthesize:

    @synthesize finished  = _finished;
    @synthesize executing = _executing;
   

   And then I implement manual setters (but rely upon the synthesized getters and ivars):

   - (void)setExecuting:(BOOL)executing
   {
       if (_executing != executing) {
           [self willChangeValueForKey:@"isExecuting"];
           _executing = executing;
           [self didChangeValueForKey:@"isExecuting"];
       }
   }
   
   - (void)setFinished:(BOOL)finished
   {
       if (_finished != finished) {
           [self willChangeValueForKey:@"isFinished"];
           _finished = finished;
           [self didChangeValueForKey:@"isFinished"];
       }
   }
   

   But if you do that, you can now just set self.executing = FALSE (or whatever) and it (a) does appropriate KVO, saving you from littering your code with all sorts of KVO calls; and (b) but saves you from having to implement properties and getters manually.