Subclassing NSOperation to internet operations with retry

Question

I'm subclassing NSOperation for http post in background thread. Those specific http posts doesn't require any value to return.

What I'm trying to do is when I've an error or timeout I want it to send after an increasing delay (fibonacci).

So far I've done this:

NSInternetOperation.h:

#import <Foundation/Foundation.h>

@interface NSInternetOperation : NSOperation
@property (nonatomic) BOOL executing;
@property (nonatomic) BOOL finished;
@property (nonatomic) BOOL completed;
@property (nonatomic) BOOL cancelled;
- (id)initWebServiceName:(NSString*)webServiceName andPerameters:(NSString*)parameters;
- (void)start;
@end

NSInternetOperation.m:

#import "NSInternetOperation.h"

static NSString * const kFinishedKey = @"isFinished";
static NSString * const kExecutingKey = @"isExecuting";

@interface NSInternetOperation ()
@property (strong, nonatomic) NSString *serviceName;
@property (strong, nonatomic) NSString *params;
- (void)completeOperation;
@end

@implementation NSInternetOperation

- (id)initWebServiceName:(NSString*)webServiceName andPerameters:(NSString*)parameters
{
    self = [super init];
    if (self) {
        _serviceName = webServiceName;
        _params = parameters;
        _executing = NO;
        _finished = NO;
        _completed = NO;
    }
    return self;
}

- (BOOL)isExecuting { return self.executing; }
- (BOOL)isFinished { return self.finished; }
- (BOOL)isCompleted { return self.completed; }
- (BOOL)isCancelled { return self.cancelled; }
- (BOOL)isConcurrent { return YES; }

- (void)start
{
    if ([self isCancelled]) {
        [self willChangeValueForKey:kFinishedKey];
        self.finished = YES;
        [self didChangeValueForKey:kFinishedKey];
        return;
    }

    // If the operation is not cancelled, begin executing the task
    [self willChangeValueForKey:kExecutingKey];
    self.executing = YES;
    [self didChangeValueForKey:kExecutingKey];

    [self main];
}

- (void)main
{
    @try {
        //
        // Here we add our asynchronized code
        //
        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
            NSURL *completeURL = [NSURL URLWithString:[NSString stringWithFormat:@"%@/%@", kWEB_SERVICE_URL, self.serviceName]];
            NSData *body = [self.params dataUsingEncoding:NSUTF8StringEncoding];
            NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:completeURL];
            [request setHTTPMethod:@"POST"];
            [request setValue:kAPP_PASSWORD_VALUE forHTTPHeaderField:kAPP_PASSWORD_HEADER];
            [request setHTTPBody:body];
            [request setValue:[NSString stringWithFormat:@"%lu", (unsigned long)body.length] forHTTPHeaderField:@"Content-Length"];
            [request setValue:@"application/x-www-form-urlencoded" forHTTPHeaderField:@"Content-Type"];


            if (__iOS_7_AND_HIGHER)
            {
                NSURLSessionConfiguration *configuration = [NSURLSessionConfiguration defaultSessionConfiguration];
                NSURLSession *session = [NSURLSession sessionWithConfiguration:configuration delegate:[Netroads sharedInstance] delegateQueue:[NSOperationQueue new]];
                NSURLSessionDataTask *dataTask = [session dataTaskWithRequest:request completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) {
                    if (error)
                    {
                        NSLog(@"%@ Error: %@", self.serviceName, error.localizedDescription);
                    }
                    else
                    {
                        //NSString *responseXML = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
                        //NSLog(@"\n\nResponseXML(%@):\n%@", webServiceName, responseXML);
                    }
                }];
                [dataTask resume];
            }
            else
            {
                [NSURLConnection sendAsynchronousRequest:request queue:[NSOperationQueue new] completionHandler:^(NSURLResponse *response, NSData *data, NSError *connectionError) {
                    if (connectionError)
                    {
                        NSLog(@"%@ Error: %@", self.serviceName, connectionError.localizedDescription);
                    }
                    else
                    {
                        //NSString *responseXML = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
                        //NSLog(@"\n\nResponseXML(%@):\n%@", webServiceName, responseXML);
                    }
                }];
            }
        });

        [self completeOperation];
    }
    @catch (NSException *exception) {
        NSLog(@"%s exception.reason: %@", __PRETTY_FUNCTION__, exception.reason);
        [self completeOperation];
    }
}

- (void)completeOperation
{
    [self willChangeValueForKey:kFinishedKey];
    [self willChangeValueForKey:kExecutingKey];

    self.executing = NO;
    self.finished = YES;

    [self didChangeValueForKey:kExecutingKey];
    [self didChangeValueForKey:kFinishedKey];
}

@end

Answer 1

A couple of reactions:

1. Before you tackle the retry logic, you should probably move your call to [self completeOperation] to inside the completion block of the NSURLSessionDataTask or sendAsynchronousRequest. Your current operation class is completing prematurely (and therefore would not honor dependencies and your network operation queue's intended maxConcurrentOperationCount).

2. The retry logic seems unremarkable. Perhaps something like:

   - (void)main
   {
       NSURLRequest *request = [self createRequest]; // maybe move the request creation stuff into its own method
   
       [self tryRequest:request currentDelay:1.0];
   }
   
   - (void)tryRequest:(NSURLRequest *)request currentDelay:(NSTimeInterval)delay
   {
       [NSURLConnection sendAsynchronousRequest:request queue:[self networkOperationCompletionQueue] completionHandler:^(NSURLResponse *response, NSData *data, NSError *connectionError) {
   
           BOOL success = NO;
   
           if (connectionError) {
               NSLog(@"%@ Error: %@", self.serviceName, connectionError.localizedDescription);
           } else {
               if ([response isKindOfClass:[NSHTTPURLResponse class]]) {
                   NSInteger statusCode = [(NSHTTPURLResponse *)response statusCode];
                   if (statusCode == 200) {
                       // parse XML response here; if successful, set `success` to `YES`
                   }
               }
           }
   
           if (success) {
               [self completeOperation];
           } else {
               dispatch_time_t popTime = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay * NSEC_PER_SEC));
               dispatch_after(popTime, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^(void){
                   NSTimeInterval nextDelay = [self nextDelayFromCurrentDelay:delay];
                   [self tryRequest:request currentDelay:nextDelay];
               });
           }
       }];
   }
   

3. Personally, I'm wary about this entire endeavor. It strikes me that you should be employing logic conditional upon the type of error. Notably, if the error is a failure resulting from lacking of internet connectivity, you should use Reachability to determine connectivity and respond to notifications to retry automatically when connectivity is restored, not simply retrying at prescribed mathematical progression of retry intervals.

   Other than network connectivity (which is better addressed with Reachability), I'm unclear as to what other network failures warrant a retry logic.

Some unrelated observations:

1. Note, I eliminated the dispatch_async of the issuing of the request in main to a background queue because you're using asynchronous methods already (and even if you weren't, you've presumably added this operation to a background queue, anyway).

2. I've also removed the try/catch logic because, unlike other languages/platforms, exception handling is not the preferred method of handling runtime errors. Typically runtime errors in Cocoa are handled via NSError. In Cocoa, exceptions are generally used solely to handle programmer errors, but not to handle the runtime errors that a user would encounter. See Apple's discussion Dealing with Errors in the Programming with Objective-C guide.

3. You can get rid of your manually implemented isExecuting and isFinished getter methods if you just define the appropriate getter method for your properties during their respective declarations:

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

4. You might, though, want to write your own setExecuting and setFinished setter methods, which do the notification for you, if you want, e.g.:

   @synthesize finished  = _finished;
   @synthesize executing = _executing;
   
   - (void)setExecuting:(BOOL)executing
   {
       [self willChangeValueForKey:kExecutingKey];
       _executing = executing;
       [self didChangeValueForKey:kExecutingKey];
   }
   
   - (void)setFinished:(BOOL)finished
   {
       [self willChangeValueForKey:kFinishedKey];
       _finished = finished;
       [self didChangeValueForKey:kFinishedKey];
   }
   

   Then, when you use the setter it will do the notifications for you, and you can remove the willChangeValueForKey and didChangeValueForKey that you have scattered about your code.

5. Also, I don't think you need to implement isCancelled method (as that's already implemented for you). But you really should override a cancel method which calls its super implementation, but also cancels your network request and completes your operation. Or, instead of implementing cancel method, you could move to the delegate based rendition of the network requests but make sure you check for [self isCancelled] inside the didReceiveData method.

   And isCompleted strikes me as redundant with isFinished. It seems like you could entirely eliminate completed property and isCompleted method.

6. You're probably unnecessarily duplicating the amount of network code by supporting both NSURLSession and NSURLConnection. You can do that if you really want, but they assure us that NSURLConnection is still supported, so it strikes me as unnecessary (unless you wanted to enjoy some NSURLSession specific features for iOS 7+ devices, which you're not currently doing). Do whatever you want, but personally, I'm using NSURLConnection where I need to support earlier iOS versions, and NSURLSession where I don't, but I wouldn't be inclined to implement both unless there was some compelling business requirement to do so.

Answer 2

Your method:

static NSString * const kFinishedKey = @"isFinished";
static NSString * const kExecutingKey = @"isExecuting";

- (void)completeOperation
{
    [self willChangeValueForKey:kFinishedKey];
    [self willChangeValueForKey:kExecutingKey];

    self.executing = NO;
    self.finished = YES;

    [self didChangeValueForKey:kExecutingKey];
    [self didChangeValueForKey:kFinishedKey];
}

Is manually sending notifications for the key paths "isFinished" and "isExecuting". NSOperationQueue observes the key paths "finished" and "executing" for those states - "isFinished" and "isExecuting" are the names of the get (read) accessors for those properties.

For an NSOperation subclass KVO notifications should be sent automatically unless your class has opted out of automatic KVO notifications by implementing +automaticallyNotifiesObserversForKey or +automaticallyNotifiesObserversOf<Key> to return NO. You can see this demonstrated in a sample project here.

Your property declarations:

@property (nonatomic) BOOL executing;
@property (nonatomic) BOOL finished;
@property (nonatomic) BOOL cancelled;

Are overriding those in NSOperation without providing the correct get accessor. Change these to:

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

To get the correct behavior for an NSOperation. NSOperation declares these as readonly in the public interface, you have the option of making them readwrite in a private class extension.

As far as implementing a connection with retry logic, there is an excellent Apple sample code project that demonstrates this, MVCNetworking