In Objective-C when implementing a method that calls a completion handler after a background task is finished how to determine thread safety?

Question

I have recently modified an existing project and modified some methods from returning a value (BOOL) to call a completion block. I followed this very good answer:

https://stackoverflow.com/a/16324193/942165

Now I have my method that calls the completion block, it is working great, but I am afraid it can unpredictably fail for not being thread safe.

I have my declaration of the block:

typedef void(^myCompletion)(id, BOOL);

Following is my method with completion handler:

-(void)myMethodThatTakesAnArgument:(id)object completionHandler:(myCompletion)completionblock {
    //do things that are allowed in the main thread:
    //...
    //...
    dispatch_queue_t backgroundQueue =
    dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0);
    dispatch_async(backgroundQueue, ^{

        [self doWorkButReturnImmediately];
        BOOL workIsNotFinished = [self isJobFinished];

        NSDate *processingStart = [NSDate date];
        NSTimeInterval timeElapsed = 0.0;
        while (workIsNotFinished && timeElapsed < 15.0) {
            [NSThread sleepForTimeInterval:1.0];
            timeElapsed = [[NSDate date] timeIntervalSinceDate:processingStart];
            workIsNotFinished = [self isJobFinished];
        }

      dispatch_async(dispatch_get_main_queue(), ^{
          // Return Result
          completionblock(object,YES);
      });
  });   
}

Following is my caller method:

- (void)callerMethod {

    NSArray *arrayOfObjects = [self prepareArrayOfObjects];

    NSMutableArray *unprocessedObjects = [arrayOfObjects mutableCopy];

    for (NSString *string in arrayOfObjects) {

        [self myMethod:string completionblock:^(id obj, BOOL success) {
        
        [unprocessedObjects removeObject:string];
        if(success){
            // do something with obj
            NSLog(@"success");
        }
        
        if ([unprocessedObjects count] == 0) {
        
            dispatch_async(dispatch_get_main_queue(), ^{
            
                // Everything is done, act accordingly.
            
            });
        
            }
        
        }
  }
}

I suspect this scenario can somehow fail and I am thinking of adding some thread safety code. I am not a lot expert of the subject, but it appears to me that probably, @synchronized could be the way to go. So I would like to embed the code of the called method in some @synchronized statements, but I am not sure if this is necessary in this case, and, if it is, I am not sure on where to specifically put the statements, I think probably in the part that is dispatched in a background queue. In the code, I have included a simple object that is passed back in the completion handler that could act as the object to pass to @synchronized. Any help is greatly appreciated. Thanks

Answer 1

The only thread-safe issue that jumps out at me is isJobFinished. You're calling it repeatedly from a second thread, so its implementation would have to be thread safe.

However, this is NOT the way to check for completion. Just remember this rule: Polling bad. Semaphores good.

This is a much cleaner, and more efficient, implementation and avoids any issue with isJobFinished:

{
    dispatch_semaphore_t jobFinishedSemaphore = dispatch_semaphore_create(0);
    dispatch_queue_t backgroundQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0);
    
    dispatch_async(backgroundQueue, ^{
        [self doWorkSignalingWhenDone:jobFinishedSemaphore];
        
        if ( dispatch_semaphore_wait(jobFinishedSemaphore, dispatch_time(DISPATCH_TIME_NOW, 15*NSEC_PER_SEC)) ) {
            // work timed out
        } else {
            // work successfully completed
            dispatch_async(dispatch_get_main_queue(), ^{
                // Return Result
                completionblock(self,YES);
            });
        }
    });
}

- (void)doWorkSignalingWhenDone:(dispatch_semaphore_t)finishedSemaphore
{
    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT,0), ^{
        // do some work in the background
        // ...
        // signal work is finished
        dispatch_semaphore_signal(finishedSemaphore);
    });
}

If modifying doWorkButReturnImmediately isn't practical, but isJobFinished is observable, you can do something similar using a value observer. In the observer method, signal the semaphore when the value of isJobFinished changes from NO to YES.

The best solution for passing jobFinishedSemaphore to the observer method would be to make it an instance variable of the object. However, that means you could only be running one of these tasks at a time. If must have multiple jobs in flight at once, or you just cannot edit the vars in the class, this should work:

{
    dispatch_semaphore_t jobFinishedSemaphore = dispatch_semaphore_create(0);
    dispatch_queue_t backgroundQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0);

    [self addObserver:self
           forKeyPath:@"isJobFinished"
              options:(NSKeyValueObservingOptionNew|NSKeyValueObservingOptionOld)
              context:(__bridge void * _Nullable)(jobFinishedSemaphore)];   /* WARNING! not retained */

    dispatch_async(backgroundQueue, ^{
        [self doWorkButReturnImmediately];
        
        if ( dispatch_semaphore_wait(jobFinishedSemaphore, dispatch_time(DISPATCH_TIME_NOW, 15*NSEC_PER_SEC)) ) {
            // work timed out
        } else {
            // work successfully completed
            dispatch_async(dispatch_get_main_queue(), ^{
                // Return Result
                completionblock(object,YES);
            });
        }
        
        // Note: this is "mostly" thread safe because this block still retains a reference to
        //  jobFinishedSemaphore, so that semephore has not been destroyed yet.
        // Remove the value observer before the reference to jobFinishedSemaphore goes out of scope.
        [self removeObserver:self forKeyPath:@"isJobFinished" context:(__bridge void * _Nullable)(jobFinishedSemaphore)];
        // at this point jobFinishedSemaphore goes out of scope, but the observer has been removed so it
        //  should no longer get sent with the (now invalid void*) reference to jobFinishedSemaphore.
    });
}

- (void)observeValueForKeyPath:(NSString*)keyPath
                      ofObject:(id)object
                        change:(NSDictionary*)change
                       context:(void*)context
{
    if ([keyPath isEqualToString:@"isJobFinished"])
        {
        if (   ![change[NSKeyValueChangeOldKey] boolValue]  // value was NO
            && [change[NSKeyValueChangeNewKey] boolValue] ) // value now YES
            {
            dispatch_semaphore_signal((__bridge dispatch_semaphore_t _Nonnull)(context));
            }
        }
}

The scary part of this code is that you're passing a non-retained copy of the semaphore pointer to the observer. But as long as you remove the observer before the semaphore is destroyed, you should be OK.