NSOperationQueue - Getting Completion Call Too Early

Question

I am using a NSOperationQueue to queue and call a number of Geocoding location lookups. I want to call a completion method when all asynchronicly running lookups have been finished.

-(void)geocodeAllItems {

    NSOperationQueue *geoCodeQueue = [[NSOperationQueue alloc]init];
    [geoCodeQueue setName:@"Geocode Queue"];

    for (EventItem *item in [[EventItemStore sharedStore] allItems]) {
        if (item.eventLocationCLLocation){
            NSLog(@"-Location Saved already. Skipping-");
            continue;
        }

        [geoCodeQueue addOperationWithBlock:^{

            NSLog(@"-Geocode Item-");
            CLGeocoder* geocoder = [[CLGeocoder alloc] init];
            [self geocodeItem:item withGeocoder:geocoder];

        }];
    }

    [geoCodeQueue addOperationWithBlock:^{
        [[NSOperationQueue mainQueue]addOperationWithBlock:^{
            NSLog(@"-End Of Queue Reached!-");
        }];
    }];


}

- (void)geocodeItem:(EventItem *)item withGeocoder:(CLGeocoder *)thisGeocoder{

    NSLog(@"-Called Geocode Item-");
    [thisGeocoder geocodeAddressString:item.eventLocationGeoQuery completionHandler:^(NSArray *placemarks, NSError *error) {
        if (error) {
            NSLog(@"Error: geocoding failed for item %@: %@", item, error);
        } else {

            if (placemarks.count == 0) {
                NSLog(@"Error: geocoding found no placemarks for item %@", item);
            } else {
                if (placemarks.count > 1) {
                    NSLog(@"warning: geocoding found %u placemarks for item %@: using the first",placemarks.count,item);
                }
                NSLog(@"-Found Location. Save it-");
                CLPlacemark* placemark = placemarks[0];
                item.eventLocationCLLocation = placemark.location;
                [[EventItemStore sharedStore] saveItems];
            }
        }
    }];
}

Output

[6880:540b] -Geocode Item-
[6880:110b] -Geocode Item-
[6880:540b] -Called Geocode Item-
[6880:110b] -Called Geocode Item-
[6880:110b] -Geocode Item-
[6880:540b] -Geocode Item-
[6880:110b] -Called Geocode Item-
[6880:540b] -Called Geocode Item-
[6880:110b] -Geocode Item-
[6880:580b] -Geocode Item-
[6880:1603] -Geocode Item-
[6880:110b] -Called Geocode Item-
[6880:1603] -Called Geocode Item-
[6880:580b] -Called Geocode Item-
[6880:907] -End Of Queue Reached!-
[6880:907] -Found Location. Save it-
[6880:907] -Found Location. Save it-
[6880:907] -Found Location. Save it-
[6880:907] -Found Location. Save it-
[6880:907] -Found Location. Save it-
[6880:907] -Found Location. Save it-
[6880:907] -Found Location. Save it-

As you can see the End of Queue function is called before the actual end of all geocoding processes + saving events. "End of Queue Reached" should only be displayed at the very end when all queued lookups have been processed. How can I get this into the right order?

Where does `-Called Geocode Item-` come from? I don't see it in the code. — Fls'Zen, Jan 02 '13 at 17:27
You can add an observer on the queue. A good answer is here http://stackoverflow.com/questions/1049001/get-notification-when-nsoperationqueue-finishes-all-tasks — Srikanth, Jan 02 '13 at 17:28
I updated the code. I did a quick clean up before posting and missed that part. — Bernd, Jan 02 '13 at 17:33
@BerndPlontsch I don't know what your final solution was, but I ended up concluding that if I wanted to use `NSOperationQueue`, I needed two operations per geocode request, one for the initial request, one for the completion handler, and I made each request dependent upon the previous request's completion handler. See http://stackoverflow.com/questions/14195706/multiple-locations-in-google-map-mkmapitem/14198584#14198584 — Rob, Jan 07 '13 at 15:12

score 7 · Accepted Answer · answered Jan 02 '13 at 20:06

Several issues are coming up here. For one, geocodeAddressString: is asynchronous, so it is returning immediately and the block operation is ending, allowing the next one to start right away. Second, you should not be making multiple calls to geocodeAddressString: one right after the other. From Apple's docs for this method:

After initiating a forward-geocoding request, do not attempt to 
initiate another forward-or reverse-geocoding request.

Third, you haven't set a max number of concurrent operations on your NSOperationQueue, so multiple blocks may be executing at once anyway.

For all of these reasons, you might want to use some GCD tools to track your calls to geocodeAddressString:. You could do this with a dispatch_semaphore (to make sure one finishes before the other starts) and a dispatch_group (to make sure you know when all of them have finished) -- something like the following. Let's assume you've declared these properties:

@property (nonatomic, strong) NSOperationQueue * geocodeQueue;
@property (nonatomic, strong) dispatch_group_t geocodeDispatchGroup;
@property (nonatomic, strong) dispatch_semaphore_t geocodingLock;

and initialized them like this:

self.geocodeQueue = [[NSOperationQueue alloc] init];
[self.geocodeQueue setMaxConcurrentOperationCount: 1];
self.geocodeDispatchGroup = dispatch_group_create();
self.geocodingLock = dispatch_semaphore_create(1);

You could do your geocoding loop like this (I've altered the code a bit to make the key parts more obvious):

-(void) geocodeAllItems: (id) sender
{
    for (NSString * addr in @[ @"XXX Address 1 XXX", @"XXX Address 2 XXX", @"XXX Address 3 XXXX"]) {
        dispatch_group_enter(self.geocodeDispatchGroup);
        [self.geocodeQueue addOperationWithBlock:^{
            NSLog(@"-Geocode Item-");
            dispatch_semaphore_wait(self.geocodingLock, DISPATCH_TIME_FOREVER);
            [self geocodeItem: addr withGeocoder: self.geocoder];
        }];
    }
    dispatch_group_notify(self.geocodeDispatchGroup, dispatch_get_main_queue(), ^{
        NSLog(@"- Geocoding done --");
    });
}

- (void)geocodeItem:(NSString *) address withGeocoder:(CLGeocoder *)thisGeocoder{

    NSLog(@"-Called Geocode Item-");
    [thisGeocoder geocodeAddressString: address completionHandler:^(NSArray *placemarks, NSError *error) {
        if (error) {
            NSLog(@"Error: geocoding failed for item %@: %@", address, error);
        } else {
            if (placemarks.count == 0) {
                NSLog(@"Error: geocoding found no placemarks for item %@", address);
            } else {
                if (placemarks.count > 1) {
                    NSLog(@"warning: geocoding found %u placemarks for item %@: using the first",placemarks.count, address);
                }
                NSLog(@"-Found Location. Save it:");
            }
        }
        dispatch_group_leave(self.geocodeDispatchGroup);
        dispatch_semaphore_signal(self.geocodingLock);
    }];
}

If, as you say, one shouldn't have concurrent geocoding requests going at the same time, doesn't that suggest a far simpler solution? Why not just have a mutable array of locations that need geocoding, and have `geocodeItem` just grab the first item, remove it from the array, geocode it, and in its completion block, just queue the next geocode request, if any, and if not, you know that they're all done? — Rob, Jan 02 '13 at 21:04
Good point! In effect this is enforcing a serial implementation -- truly serial, because it waits for the completion block, unlike a simple NSOperationQueue -- and this could definitely be done in other, less complex ways. I do think the serial aspect is important, however it's implemented; my test code did not work when the calls were launched concurrently but this code worked fine. — Peter E, Jan 02 '13 at 21:18
@Rob You nailed it right there. I checked the documentation and Peter is correct it is one at a time. The real solution is quite simple. — Samuel, Jan 02 '13 at 22:58

score 4 · Answer 2 · edited Jan 02 '13 at 18:24

4

A good solution is to add all geocoding operations as dependencies of the final cleanup operation:

- (void)geocodeAllItems {
    NSOperationQueue *geoCodeQueue = [[NSOperationQueue alloc] init];

    NSOperation *finishOperation = [NSBlockOperation blockOperationWithBlock:^{
        // ...
    }];

    for (EventItem *item in [[EventItemStore sharedStore] allItems]) {
        // ...
        NSOperation *operation = [NSBlockOperation blockOperationWithBlock:^{
            // ...
        }];
        [finishOperation addDependency:operation]
        [geoCodeQueue addOperation:operation];
    }

    [geoCodeQueue addOperation:finishOperation];
}

Another solution would be to make the operation queue serial. The operations are still performed on a background thread, but only one at a time and in the order in which they are added to the queue:

NSOperationQueue *geoCodeQueue = [[NSOperationQueue alloc] init];
[geoCodeQueue setMaxConcurrentOperationCount:1];

edited Jan 02 '13 at 18:24

matt

515,959
87
875
1,141

answered Jan 02 '13 at 17:40

Nikolai Ruhe

81,520
17
180
200

1

This looks good to me. However I still get the "End of Queue Reached" message too early - before the "Found Location. Save it" message. Could it be an issue with the actual geocoding happening in another block and therefore escaping the control scope of the NSOperationQueue? – Bernd Jan 02 '13 at 18:44
4

@BerndPlontsch Yes, it absolutely could cause this problem, because `CLGeocoder`, itself, operates asynchronously. When using this dependency trick, the dependent operations must, individually, operate synchronously. You'll have to keep track of pending `geocodeAddressString` requests, so it will therefore have to (a) wait for Nikolai's completionOperation to fire (meaning that all of the requests have been submitted); and only then (b) then wait to see if all of the requests are done. – Rob Jan 02 '13 at 18:48

Samuel · Answer 3 · 2013-01-02T19:20:43.990

3

CompletionBlocks are built in to NSOperation and NSBlockOperation can handle multiple blocks so it's really easy to just add all the work that you need to run async and set your completion block to be called when it is all finished.

- (void)geocodeAllItems {
    NSOperationQueue *geoCodeQueue = [[NSOperationQueue alloc] init];

    NSBlockOperation *operation = [[[NSBlockOperation alloc] init] autorelease]

    for (EventItem *item in [[EventItemStore sharedStore] allItems]) {
        // ...
        // NSBlockOperation can handle multiple execution blocks
        operation addExecutionBlock:^{
            // ... item ...
        }];
    }

    operation addCompletionBlock:^{
         // completion code goes here
         // make sure it notifies the main thread if need be.
    }];

    // drop the whole NSBlockOperation you just created onto your queue
    [geoCodeQueue addOperation:operation];
}

Note: You can't assume that the operations will be performed in your geoCodeQueue. They will be run concurrently. NSBlockOperation manages this concurrency.

edited Jan 02 '13 at 19:20

answered Jan 02 '13 at 18:49

Samuel

401
2
10

1

That's like adding individual operations to a serial queue: All operations are performed in order, not concurrently. – Nikolai Ruhe Jan 02 '13 at 18:54
2

They are performed concurrently per the documentation: https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/NSBlockOperation_class/Reference/Reference.html – Samuel Jan 02 '13 at 19:17
1

Oh, I see, I was under the wrong assumption that the blocks are performed serially. Thanks for teaching me something new! +1 – Nikolai Ruhe Jan 02 '13 at 19:32
1

I have to confess that I shared Nikolai's skepticism about this technique, so I just tested it. First, I think `addCompletionBlock` should be `setCompletionBlock`. Secondly, and more importantly, unlike adding distinct `NSOperationBlock` objects to the queue, this doesn't seem to honor the queue's `maxConcurrentOperationCount`. When I set `maxConcurrentOperationCount` to `4` or `6`, it works as advertised when adding distinct `NSOperationBlock` objects, but when I simply `addExecutionBlock` to a single `NSBlockOperation`, it appears to only run two concurrently. Very curious. – Rob Jan 02 '13 at 19:57
1

@Rob I concur. I did the same testing to make sure the blocks are run concurrently. Even on a serial queue (maxConcurrentOperationCount = 1) the blocks are performed in parallel. – Nikolai Ruhe Jan 02 '13 at 20:27

score 1 · Answer 4 · answered Jan 02 '13 at 17:37

1

NSOperationQueue doesn't work in the way you think, there is no direct dependence between the execution order and adding order. You can call the function in which you subtract till the number equals to zero and you can call the "callback" function.

answered Jan 02 '13 at 17:37

Paul Nadolinskyi

114
10

1

I'm not sure it's true that there's _no_ correlation between execution order that you add them and the order that they're executed. They're always initiated in order, but due to concurrency, there may be some still finishing as the last one finally kicks off. If `maxConcurrentOperationCount` was `1` (i.e. it was a serial queue), it would behave as the OP expected. But Nikolai suggests an elegant solution for dependencies between queue operations in a concurrent queue. – Rob Jan 02 '13 at 18:32

score 1 · Answer 5 · answered Jan 02 '13 at 17:42

NSOperationQueues run multiple operations concurrently by default. In practice of course, that means that operations added to the queue will not necessarily be started or finished in the same order you added them.

You can make the queue run all operations serially by setting the queue's maxConcurrentOperationCount value to 1 after you create it:

NSOperationQueue *geoCodeQueue = [[NSOperationQueue alloc]init];
[geoCodeQueue setName:@"Geocode Queue"];
[geoCodeQueue setMaxConcurrentOperationCount:1];

If you do indeed want operations to run concurrently, but still want to be notified when they've all finished, observe the queue's operations property and wait until it reaches zero, as explained in answer linked to by Srikanth in his comment.

EDIT: Nikolai Ruhe's answer is great too.

+1 for pointing out that serial queue would operate as OP expected. But for concurrent queues, I always use Nikolai's approach. It enjoys greater concurrency but honors the OP's desired dependencies. +1 for both of you. — Rob, Jan 02 '13 at 18:35

NSOperationQueue - Getting Completion Call Too Early

5 Answers5

Linked