How to improve this objective-c code (blocks, RestKit, async, threads)

Question

I'm maintaining an old game code (>5 yrs old) and switched developers hands a few times. Game doesn't has a dedicated player base (an early casino gambling game).

RestKit is used for API calls.

Please find comments: // SECTION_1 // SECTION_2 in the code below.

// SECTION_1 : can make it async, use blocking logic. What are the some immediate risks related to introducing threading bugs?

// SECTION_2 : Need to fix a bug bug in previous logic here. Bug: self.fetchAllPlayersCallback gets invoked before waiting for self.fetchAllPlayersFriendCheckCallback. For correct UI update, I would need to combine self.fetchAllPlayersFriendCheckCallback and self.fetchAllPlayersCallback.

Code:

/* getAllPlayersInGame:(NSString *)gameId 
 *  Fetch players for a game in progress, update UI, invoke fetchAllPlayersCallback
 *  Also detect if players are friends. Prepare friends set and invoke fetchAllPlayersFriendCheckCallback.
 */  
- (void)getAllPlayersInGame:(NSString *)gameId
{
    self.fetchAllPlayersInProgress = YES;
    self.fetchAllPlayersError = nil;
    [SocialManager getPlayersAndProfilesForGameId:gameId userId:[UserManager getActiveUser] completion:^(NSError *error, SocialUsers *users, SocialProfiles *profiles) 
    {
        if (error) {
            self.fetchAllPlayersError = error;
            // TODO: show ui error alert
            return;
        }

        __block NSUInteger totalusers = [self.lobby.players count];        
        __block BOOL isAllPlayersFriends = YES;
        __block NSMutableSet *friendsInGame = [[NSMutableSet alloc] init]

        // SECTION_1
        // separate lightweight call to server per player. 
        // server implementation limitation doesn't allow sending bulk requests.            
        for (SocialUser *player in self.lobby.players) {
            NSString *playerId = player.playerID;

            [SocialManager isUser:userId friendsWithPlayer:playerId completionBlock:^(PlayHistory *playHistory, NSError *error) {
                totalusers--;                                
                if (!error) {
                    isAllPlayersFriends &= playHistory.isFriend;
                    if (playHistory.isFriend) 
                    {
                        // TODO: Add to friendsInGame
                        // TODO: save other details (game history, etc for ui population)
                    }                    
                } else {
                    self.fetchAllPlayersFriendCheckCallback(isAllPlayersFriends, friendsInGame, error);
                    return;
                }

                if (0 == totalusers) {
                    fetchAllPlayersFriendCheckCallback(isAllPlayersFriends, friendsInGame, error);
                }
            }];
        };

        // SECTION_2
        // TODO: update data model        
        // TODO: UI update view
        self.fetchAllPlayersInProgress = NO;
        if (self.fetchAllPlayersCallback) 
        {
            self.fetchAllPlayersCallback();
            self.fetchAllPlayersCallback = nil;
        }
    }];
}

Answer 1

There are a few approaches:

1. If you have a bunch of asynchronous requests that can happen concurrently with respect to each other and you want to trigger some other task when they're done, you might use Grand Central Dispatch (GCD) dispatch groups.

   For example, rather than counting down totalUsers, the standard GCD approach is to use a dispatch group. Dispatch groups can trigger some block that will be called when a bunch of asynchronous calls are done. So you:

   • Create a group before you start your loop;
   • Enter your group before you start asynchronous call;
   • Leave your group in the asynchronous call's completion handler;
   • Specify a dispatch_group_notify block that will be called when each "enter" is matched with a "leave".
      

   Thus, something like:

   dispatch_group_t group = dispatch_group_create();
   
   for (SocialUser *player in self.lobby.players) { 
       dispatch_group_enter(group);
   
       [SocialManager ...: ^{
           ...
           dispatch_group_leave(group);
       }];
   }
   
   dispatch_group_notify(group, dispatch_get_main_queue(), ^{
       fetchAllPlayersFriendCheckCallback(isAllPlayersFriends, friendsInGame, error);
   
       self.fetchAllPlayersInProgress = NO;
       if (self.fetchAllPlayersCallback) {
           self.fetchAllPlayersCallback();
           self.fetchAllPlayersCallback = nil;
       }
   });
   

   Now, this presumes that this call is asynchronous but that they can run concurrently with respect to each other.

2. Now, if these asynchronous calls need to be called consecutively (rather than concurrently), then you might wrap them in asynchronous NSOperation or something like that, which assures that even if they're running asynchronously with respect to the main queue, they'll run consecutively with respect to each other. And if you use that approach, rather than using a dispatch group for the completion operations, you would use NSOperation dependencies. For example, here's a trivial example:

   NSOperationQueue *queue = [[NSOperationQueue alloc] init];
   queue.maxConcurrentOperationCount = 1;
   
   NSOperation *completion = [NSBlockOperation blockOperationWithBlock:^{
       // stuff to be done when everything else is done
   }];
   
   for (Foo *foo in self.foobars) {
       NSOperation *operation = [SocialManager operationForSomeTask:...];
       [completionOperation addDependency:operation];
       [queue addOperation:operation];
   }
   
   [[NSOperationQueue mainQueue] addOperation:completionOperation];
   

   But all of this assumes that you're refactored your social manager to wrap its asynchronous requests in custom asynchronous NSOperation subclass. It's not rocket science, but if you haven't done that before, you might want to gain familiarity with creating them before you tackle refactoring your existing code to do so.

3. Another permutation of the previous point is that rather than refactoring your code to use custom asynchronous NSOperation subclasses, you could consider a framework like PromiseKit. It still requires you to refactor your code, but it has patterns that let you wrap your asynchronous task in "promises" (aka "futures"). I only mention it for the take of completeness. But you might not want to throw a whole new framework in this mix.

Bottom line, there's simply not enough here to diagnose this. But dispatch groups or custom asynchronous NSOperation subclasses with completion operations.

But the comment in that code that says "use blocking logic" is generally not a good idea. You should never block and with well designed code, it's completely unnecessary.