Why won't this loop exit

Question

My assumption is that the operations are running asynchronously on a separate thread, but the loop never exits, so something is not as I assumed.

/**
 Checks if we can communicate with the APIs

 @result YES if the network is available and all of the registered APIs are responsive
 */
- (BOOL)apisAvailable
{
    // Check network reachability
    if (!_connectionAvailable) {
        return NO;
    }
    // Check API server response
    NSMutableSet *activeOperations = [[NSMutableSet alloc] init];
    __block NSInteger successfulRequests = 0;
    __block NSInteger failedRequests = 0;
    for (AFHTTPClient *httpClient in _httpClients) {
        // Send heart beat request
        NSMutableURLRequest *request = [httpClient requestWithMethod:@"GET" path:@"" parameters:nil];
        AFHTTPRequestOperation *operation = [[AFHTTPRequestOperation alloc] initWithRequest:request];
        [operation setCompletionBlockWithSuccess:^(AFHTTPRequestOperation *operation, id responseObject) {
            // Server returned good response
            successfulRequests += 1;
        } failure:^(AFHTTPRequestOperation *operation, NSError *error) {
            // Server returned bad response
            failedRequests += 1;
        }];
        [operation start];
        [activeOperations addObject:operation];
    }
    // Wait for heart beat requests to finish
    while (_httpClients.count > (successfulRequests + failedRequests)) {
        // Wait for each operation to finish, one at a time
        //usleep(150);
        [NSThread sleepForTimeInterval:0.150];
    }
    // Check final results
    if (failedRequests > 0) {
        return NO;
    }
    return YES;
}

Answer 1

I believe the issue is that I was not using locking to increment the counters so the while loop would never evaluate to true.

I was able to get it working by only looking for a fail count greater than 0 that way as long as it was incremented by any of the request callback blocks then I know what to do.

I just so happen to have switched to [NSOperationQueue waitUntilAllOperationsAreFinished].

Final code:

/**
 Checks if we can communicate with the APIs

 @result YES if the network is available and all of the registered APIs are responsive
 */
- (BOOL)apisAvailable
{
    // Check network reachability
    if (!_connectionAvailable) {
        return NO;
    }
    // Check API server response
    NSOperationQueue *operationQueue = [[NSOperationQueue alloc] init];
    __block NSInteger failedRequests = 0;
    for (AFHTTPClient *httpClient in _httpClients) {
        // Send heart beat request
        NSMutableURLRequest *request = [httpClient requestWithMethod:@"GET" path:@"" parameters:nil];
        AFHTTPRequestOperation *operation = [[AFHTTPRequestOperation alloc] initWithRequest:request];
        [operation setCompletionBlockWithSuccess:^(AFHTTPRequestOperation *operation, id responseObject) {
            // Server returned good response
        } failure:^(AFHTTPRequestOperation *operation, NSError *error) {
            // Server returned bad response
            failedRequests += 1;
        }];
        [operationQueue addOperation:operation];
    }
    // Wait for heart beat requests to finish
    [operationQueue waitUntilAllOperationsAreFinished];
    // Check final results
    if (failedRequests > 0) {
        return NO;
    }
    return YES;
}

Answer 2

A few suggestions:

• Never check reachability to determine if a request will succeed. You should try the request; only if it fails should you consult reachability to try and get a best guess as to why. Reachability makes no guarantee about whether a request will fail or succeed.

• Is this method called on the main thread? Even if you fixed the problem with the requests never completing, it will block the UI the entire time your network requests are running. Since these requests can take potentially a long time, this is a bad experience for the user as well as something the OS will kill your app for if it happens at the wrong time (e.g. at launch).

• Looping while calling sleep or equivalent is wasteful of CPU resources and memory, as well as prevents the thread's runloop from servicing any timers, event handler or callbacks. (Which is probably why the networking completion blocks never get to run.) If you can avoid blocking a thread, you should. In addition, Cocoa will very often be unhappy if you do this on an NSThread you didn't create yourself.

I see two options:

1. Use dispatch_groups to wait for all of your requests to finish. Instead of blocking your calling thread, you should instead take a completion block to call when you're done. So, instead of returning a BOOL, take a completion block which takes a BOOL. Something like - (void)determineIfAPIIsAvailable:(void(^)(BOOL))completionBlock;

2. Get rid of this method altogether. What are you using this method for? It's almost certainly a better idea to just try to use your API and report appropriate errors to the user when things fail rather than to try to guess if a request to the API will succeed beforehand.