4

I have an NSOperation with an NSOperationQueue that has a bunch of child operations, some queued up.

I had a problem where even after calling cancelAllOperations on the queue my main method was hanging on waitUntilAllOperationsAreFinished.

Then when I set the complete flag I use for isFinished upon cancellation it no longer gets backed up in a cancelled queue.

- (BOOL)isFinished
{
    return complete;
}

- (void)cancel
{
    cancelled = YES;
    complete = YES;
    [_childOperationQueue cancelAllOperations];
}

Is this the correct behaviour, a cancelled operation should be technically finished? It seems like NSOperation needs the isFinished to be set to true before it will remove it, in thought this might allow it to 'clean up', but I don't know what the protocol is here and google didn't reveal much.

James Bedford
  • 28,702
  • 8
  • 57
  • 64
Mitchell Currie
  • 2,769
  • 3
  • 20
  • 26
  • Are you sure your operation is actually cancelled? Telling it to cancel does not immediately cancel it, it just tells the block it should cancel itself. You need to actually check `self.cancelled` and abort your operation. You should not be overriding `isFinished` or `cancel` — the default behaviour for both should work. – Abhi Beckert Jun 19 '14 at 01:02
  • 2
    @AbhiBeckert If writing a non-concurrent operation, you are correct. But when writing concurrent operations, you always override `isFinished` method (either explicitly, or with automatically synthesized getter). Also, re `cancel`, while I agree that you don't generally override that, just check `isCancelled` periodically, sometimes you cannot do that. For example, if you initiate a block-based `NSURLSessionTask`, there is no where you can check `isCancelled`, so you will override `cancel` method in that scenario. – Rob Jun 19 '14 at 11:48

2 Answers2

4

Canceling the operation just sets isCancelled to return YES. This means in your NSOperation block you can check for isCancelled and prevent unnecessarily doing any work (you need to implement this logic yourself).

Your main thread needs to wait for all the operations on the queue, but if your NSOperation block checks isCancelled before doing anything you should quickly get through all the queued operations and the wait shouldn't be long.

James Bedford
  • 28,702
  • 8
  • 57
  • 64
  • Also note that if a block is cancelled but has not begun execution yet, then it will never start executing — it should just immediately change to "finished". – Abhi Beckert Jun 19 '14 at 01:06
  • 1
    Are you sure? I thought it would still be executed and it's your job to check if it was cancelled? – James Bedford Jun 19 '14 at 01:07
  • 1
    The docs seem to suggest they shouldn't be executed, but I swear in my experience they were. I wonder whether the fact that the main thread's blocked waiting for everything on the queue to finish would affect this. – James Bedford Jun 19 '14 at 01:11
  • I don't have time to test it now, but I have a queue with ~50,000 items that will usually be cancelled when it has only made it through the first 100 or so items. I'm fairly sure when I tested that cancelling did empty the queue of tasks that had not started yet. – Abhi Beckert Jun 19 '14 at 01:32
  • 1
    I've found that once I've started waiting for all operations to complete, the reason it hangs is two items were left in the queue, their status was 'cancelled'. It seems that being cancelled is not enough to be removed from a queue, it seems like it only checks isFinished, and there are no other operation loops in progress. So it looks like it doesn't start the operation once i call 'cancelAllOperations' but it won't remove them from the queue if they haven't started – Mitchell Currie Jun 19 '14 at 03:30
  • That sounds like an explanation I'd go with (implementation detail and may vary over releases). – James Bedford Jun 19 '14 at 04:46
3

If you are writing a concurrent operation (i.e. isConcurrent method returns YES), I have two observations:

  1. When you change the state of your operation to be finished, you have to manually perform the appropriate isFinished KVN. Thus, before you change your state variable, call willChangeValueForKey, and after you change it, call didChangeValueForKey.

    There are many ways to achieve this, but I define my finished and executing properties like so:

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

    And I use those synthesized getters, but write setters that do the appropriate KVN:

    @synthesize finished  = _finished;
    @synthesize executing = _executing;
    
    - (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"];
        }
    }
    

    Then, when I want to complete the operation, I call a method like so:

    - (void)completeOperation
    {
        self.executing = NO;
        self.finished = YES;
    }
    

    That results in the KVN for isExecuting and isFinished (as well as ensuring that my synthesized getters return the appropriate values, too). If you don't do this, your operations may never finish.

  2. In terms of canceling the operation, there are two approaches:

    • You can periodically check [self isCancelled] (a very common pattern if you have a loop or some method which is being called repeatedly, e.g. a didReceiveData in a delegate based connection/session), and if so, do whatever clean-up you need and then also complete the operation (the above completeOperation method).

    • If you don't have any place you can periodically check the cancellation status, you want to override the cancel method, which does whatever cleanup you need, calls [super cancel] but also ensures that the operation finishes, too (e.g. call completeOperation if the operation is executing).

    I also have start check the cancellation status:

    - (void)start
    {
        if ([self isCancelled]) {
            self.finished = YES;
            return;
        }
    
        self.executing = YES;
    
        // start my operation
    }
    

Beyond the above, it's hard to counsel you further without knowing a bit more about what's in the heart of the operation (NSURLConnection? NSURLSession? something else?).

For more information, see the Configuring Operations for Concurrent Execution in the Concurrency Programming Guide: Operation Queues.

Rob
  • 415,655
  • 72
  • 787
  • 1,044