Canonical Implementation of a Subclass of NSOperation

Question

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

• rolfl: Are you satisfied that your code is working? Are there problems you need to resolve or is the code ready for a review?
• CouchDeveloper: 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.
• Aaron Brager: 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.
• CouchDeveloper: @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.
• rolfl: @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.

Answer 1

- (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.

• CouchDeveloper: 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.