10
\$\begingroup\$

I would like to develop a kind of template or canonical implementation for a concurrent subclass of NSOperation.

Edit: there is a related request which implements the NSOperation subclass in Swift.

Requirements:

  • Thread safe API.
  • start shall only start the task once, otherwise it has no effect.
  • The NSOperation object shall be immortal while the task is executing.

References:

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSOperation_class/Reference/Reference.html


Solution

So far, I came up with this solution:

#import <Foundation/Foundation.h>

/**
 Canonical, Concurrent Subclass of NSOperation
 */

typedef void (^completion_block_t)(id result);

@interface MyOperation : NSOperation

// Designated Initializer
// For the sake of testing, parameter count equals the duration in 1/10 seconds until the task is fininshed.
- (id)initWithCount:(int)count completion:(completion_block_t)completioHandler;

@property (nonatomic, readonly) id result;
@property (nonatomic, copy) completion_block_t completionHandler;
@end

@implementation MyOperation {
    BOOL _isExecuting;
    BOOL _isFinished;
    
    dispatch_queue_t _syncQueue;
    int _count;
    id _result;
    completion_block_t _completionHandler;
    id _self;  // immortality
}

- (id)initWithCount:(int)count completion:(completion_block_t)completionHandler
{
    self = [super init];
    if (self) {
        _count = count;
        _syncQueue = dispatch_queue_create("op.sync_queue", NULL);
        _completionHandler = [completionHandler copy];
    }
    return self;
}

- (id) result {
    __block id result;
    dispatch_sync(_syncQueue, ^{
        result = _result;
    });
    return result;
}

- (void) start
{
    dispatch_async(_syncQueue, ^{
        if (!self.isCancelled && !_isFinished && !_isExecuting) {
            self.isExecuting = YES;
            _self = self; // make self immortal for the duration of the task
            dispatch_async(dispatch_get_global_queue(0, 0), ^{
                
                // Simulated work load:
                int count = _count;
                while (count > 0) {
                    if (self.isCancelled) {
                        break;
                    }
                    printf(".");
                    usleep(100*1000);
                    --count;
                }
                
                // Set result and terminate
                dispatch_async(_syncQueue, ^{
                    if (_result == nil && count == 0) {
                        _result = @"OK";
                    }
                    [self terminate];
                });
            });
        }
    });
}

- (void) terminate {
    self.isExecuting = NO;
    self.isFinished = YES;
    completion_block_t completionHandler = _completionHandler;
    _completionHandler = nil;
    id result = _result;
    _self = nil;
    if (completionHandler) {
        dispatch_async(dispatch_get_global_queue(0, 0), ^{
            completionHandler(result);
        });
    }
}


- (BOOL) isConcurrent {
    return YES;
}

- (BOOL) isExecuting {
    return _isExecuting;
}
- (void) setIsExecuting:(BOOL)isExecuting {
    if (_isExecuting != isExecuting) {
        [self willChangeValueForKey:@"isExecuting"];
        _isExecuting = isExecuting;
        [self didChangeValueForKey:@"isExecuting"];
    }
}

- (BOOL) isFinished {
    return _isFinished;
}
- (void) setIsFinished:(BOOL)isFinished {
    if (_isFinished != isFinished) {
        [self willChangeValueForKey:@"isFinished"];
        _isFinished = isFinished;
        [self didChangeValueForKey:@"isFinished"];
    }
}

- (void) cancel {
    dispatch_async(_syncQueue, ^{
        if (_result == nil) {
            NSLog(@"Operation cancelled");
            [super cancel];
            _result = [[NSError alloc] initWithDomain:@"MyOperation"
                                                 code:-1000
                                             userInfo:@{NSLocalizedDescriptionKey: @"cancelled"}];
        }
    });
}

@end
\$\endgroup\$
12
  • \$\begingroup\$ Are you satisfied that your code is working? Are there problems you need to resolve or is the code ready for a review? \$\endgroup\$
    – rolfl
    Dec 4, 2013 at 12:23
  • \$\begingroup\$ I'm quite confident that there are no obvious errors. But thread-safity is tricky. So I would appreciate it if you or anybody else can take a look and possibly spot potential issues. ;) E.g.: dead locks, due to dispatch lib, circular references, etc. \$\endgroup\$ Dec 4, 2013 at 12:36
  • 1
    \$\begingroup\$ Why aren't you dispatching on to _syncQueue for terminate and the setters & getters for isExecuting and isFinished? I guess it doesn't matter for terminate, since only you access that, but isExecuting and isFinished could be accessed by a developer from any thread. \$\endgroup\$ Jan 5, 2014 at 21:28
  • \$\begingroup\$ @AaronBrager It's guaranteed by the implementation that terminate will be executed on the _syncQueue. For properties isExecuting and isFinished, IMHO synchronization and memory barriers are not required: both are boolean values which are initially set to zero. Any access might return either zero or a non-zero value - which is the value of the corresponding memory location (no register). When a NO is returned, its value MUST be treated immediately stale, anyway. It's also guaranteed that the value of the memory location will be eventually set and never changes subsequently. \$\endgroup\$ Jan 7, 2014 at 11:03
  • 2
    \$\begingroup\$ @CouchDeveloper: Please read: What you can and cannot do after receiving answers to understand what edits can make sense when there are answers to your questions. \$\endgroup\$
    – rolfl
    Jul 6, 2014 at 13:40

1 Answer 1

6
\$\begingroup\$
- (id)initWithCount:(int)count completion:(completion_block_t)completioHandler;

There's a typo here, a missing "n" in the last argument.


- (id)initWithCount:(int)count completion:(completion_block_t)completionHandler
{

- (id) result {

- (void) start
{

This inconsistency is really bothersome to me. The opening bracket should either be on the same line or the next line, but do it consistently. Personally, I prefer the opening bracket on the same line as it looks better when the methods are collapsed in Xcode.


if (!self.isCancelled && !_isFinished && !_isExecuting) {
    self.isExecuting = YES;

This inconsistency also bothers me, and it's not immediately clear why it's done. In fact, even as a season Objective-C programmer, I'm confused as to what would actually happen here when you do self.isExecuting = YES;

Normally, you'd do self.isExecuting = YES; because you've defined isExecuting as a property, and what is actually done is more akin to this:

[self setIsExecuting:YES];

But you've not defined isExecuting as a full property, but rather you've defined _isExecuting as an instance variable and created setIsExecuting:(BOOL)isExecuting as a method.

I don't rightly know without just running the code what would or should happen here.

So the solution is to either define isExecuting as a property so it's more clear what self.isExecuting = YES; would actually do, or to change your self.isExecuting = YES; to either _isExecuting = YES; or [self setIsExecuting:YES]; depending on the intent.

\$\endgroup\$
1
  • \$\begingroup\$ The property declarations were indeed missing. Thanks for pointing this to us. Just for information: the dot syntax would still work - but I too prefer to have proper declarations ;) Then, the invocation of the property setter in the implementation for isExecuting and isFinished is required in order to fire the KVO notification messages. \$\endgroup\$ Jul 6, 2014 at 11:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.