Passing objects into NSOperation and ensuring correct memory management policies

Question

If I want to pass an object that was created on the main thread onto an NSOperation object, what's the standard way of doing to so that I'm not creating any memory management issues? Should I make my object's properties not have the 'nonatomic' attribute?

Right now, I allocate the objects via [[[AClass alloc] init] autorelease], keep a copy of the instance on my main thread and then pass another copy into the NSOperation as part of an NSArray. When I try to iterate through the array list objects inside NSOperation class and access one of the AClass's properties, the debugger reports that one of the member properties of AClass's instance object is already zombied while others are not. The error I'm seeing is:

-[CFString retain]: message sent to deallocated instance 0x5a8c6b0
*** -[CFString _cfTypeID]: message sent to deallocated instance 0x5a8c6b0
*** -[CFString _cfTypeID]: message sent to deallocated instance 0x5a8c6b0

I can't tell who is releasing my string properties too early but the entire object instance has not been released.

My class looks like:

@interface AClass
{
   NSString *myTitle;
   NSString *myDescription;
}

@property (nonatomic, retain, readonly) NSString *myTitle;
@property (nonatomic, retain, readonly) NSString *myDescription;

@end

@implementation AClass
@synthesize myTitle, myDescription;
- (void)dealloc
{ 
    [myTitle release];
    [myDescription release];
}
@end

Answer 1

If there is something I do not recommend, it is that you are keeping your reference unretained by the knowledge that it will be retained by someone else already. Because the reference is unretained, when it is removed from the array it will be released, retain count might drop to zero and object might be dealloc and now you are holding time bomb i.e. invalid reference.

I would suggest that do not autorelease the reference, and try to do break point at your dealloc and see the stack call to see who cause your object to be dealloc.

Answer 2

Here's an updated snippet for an efficient, 'thread-safe' version of AClass:

/**
    AClass is an immutable container:
      - category methods must never change the state of AClass
*/
@interface AClass : NSObject < NSCopying >
{
@private
    NSString * title;
    NSString * description;
}
/**
   subclassing notes:
     - do not override properties: title, description
     - implement @protocol NSCopying
*/

/*
1) document copy on entry here, even though the compiler has no
      additional work to do.
2) nonatomic in this case - these ivars initialized and never mutate.
3) readonly because they are readonly
*/
@property (copy, readonly, nonatomic) NSString * title;
@property (copy, readonly, nonatomic) NSString * description;

/* prohibited: */
- (id)init;

/* designated initializer */
- (id)initWithTitle:(NSString *)inTitle description:(NSString *)inDescription;

@end

@implementation AClass

@synthesize title;
@synthesize description;

- (id)init
{
    assert(0 && "use the designated initializer");
    self = [super init];
    [self release];
    return 0;
}

- (id)initWithTitle:(NSString *)inTitle description:(NSString *)inDescription
{
    self = [super init];
    assert(self && "uh oh, NSObject returned 0");

    if (0 != self) {

        if (0 == inTitle || 0 == inDescription) {
            assert(inTitle && inDescription && "AClass: invalid argument");
            [self release];
            return 0;
        }
            /* this would catch a zombie, if you were given one */
        title = [inTitle copy];
        description = [inDescription copy];

        if (0 == title || 0 == description) {
            assert(title && description && "string failed to copy");
            [self release];
            return 0;
        }

    }
    return self;
}

- (void)dealloc
{
    /* which could also happen when if your init fails, but the assertion in init will be hit first */
    assert(title && description && "my ivars are not meant to be modified");

    [title release], title = 0;
    [description release], description = 0;

    /* don't forget to call through super at the end */
    [super dealloc];
}


- (id)copyWithZone:(NSZone *)zone
{
    assert(self.title == title && self.description == description && "the subclasser should not override the accessors");

    if ([self zone] == zone && [self class] == [AClass class]) {
        /*
        this is one possible (optional) optimization:
          - avoid using this approach if you don't entirely understand
            all the outlined concepts of immutable containers and low 
            level memory management in Cocoa and just use the
            implementation in 'else'
        */
        return [self retain];
    }

    else {
        return [[[self class] allocWithZone:zone] initWithTitle:self.title description:self.description];
    }

}

@end

Beyond that, avoid overusing autorelease calls so your memory issues are local to the callsite. This approach will solve many issues (although memory issues may still exist in your app).

Update in response to questions:

Justin Galzic: so basically, the copy ensures that objects are local to the caller and when the instance is shared out to the thread on which NSOperations is on, they're two different instances?

actually, the copy call to an immutable string could perform a retain.

as an example: AClass could now implement @protocol NSCopying by simply retaining the 2 strings. also, if you know AClass is never subclassed, you could just return [self retain] when the objects are allocated in the same NSZone.

if a mutable string is passed to the initializer of AClass, then it will (of course) perform a concrete copy.

if you want objects to share these strings, then this approach is preferred (in most cases) because you (and all clients using AClass) now know the ivars will never change behind your back (what they point to, as well as the strings' contents). of course, you still have the ability to make the mistake of changing what title and description point to in the implementation of AClass - this would break the policy you've established. if you wanted to change what the members of AClass pointed to, you'd have to use locks, @synchronized directives (or something similar) - and then typically set up some observation callbacks so you could guarantee that your class works as expected... all that is unnecessary for most cases because the above immutable interface is perfectly simple for most cases.

to answer your question: the call to copy is not guaranteed to create a new allocation - it just allows several guarantees to propagate to clients, while avoiding all thread safety (and locking/synchronizing).

What if there are some cases where you do want multiple classes (on the same thread) to share this object? Would you then make an implicit copy of this object and then pass along to the NSOperation?

now that i've detailed how copying immutable objects can be implemented. it should be obvious that properties of immutable objects (NSString, NSNumber, etc.) should be declared copy in many cases (but many Cocoa programmers don't declare them that way).

if you want to share a NSString which you know is immutable, you should just copy it from AClass.

if you want to share an instance of AClass you have 2 choices:

1) (best) implement @protocol NSCopying in AClass: - (id)copyWithZone: implementation added above.

now the client is free to copy and retain AClass as is most logical for their needs.

2) (BAD) expect that all clients will keep their code up to date with changes to AClass, and to use copy or retain as required. this is not realistic. it is a good way to introduce bugs if your implementation of AClass needs to change because clients will not always update their programs accordingly. some people consider this acceptable when the object is private in a package (e.g., only one class uses and sees its interface).

in short, it's best to keep the retain and copy semantics predictable - and just hide all the implementation details in your class so your clients' code never breaks (or is minimized).

if your object is truly shared and its state is mutable, then use retain and implement callbacks for state changes. otherwise, keep it simple and use immutable interfaces and concrete copying.

if an object has an immutable state, then this example is always a lock free thread safe implementation with many guarantees.

for an implementation of an NSOperation subclass, i find it best (in most cases) to: - create an object which provides all the context it needs (e.g., an url to load) - if the something needs to know about the operation's result or to use the data, then create a @protocol interface for the callbacks and add a member to the operation subclass which is retained by the NSOperation subclass, and which you've prohibited from pointing to another object during the lifetime of the NSOperation instance:

@protocol MONImageRenderCallbackProtocol

@required
/** ok, the operation succeeded */
- (void)imageRenderOperationSucceeded:(AClass *)inImageDescriptor image:(NSImage *)image;

@required
/** bummer. the image request failed. see the @a error */
- (void)imageRenderOperationFailed:(AClass *)inImageDescriptor withError:(NSError *)error;

@end

/* MONOperation: do not subclass, create one instance per render request */
@interface MONOperation : NSOperation
{
@private
    AClass * imageDescriptor; /* never change outside initialization/dealloc */
    NSObject<MONImageRenderCallbackProtocol>* callback; /* never change outside initialization/dealloc */
    BOOL downloadSucceeded;
    NSError * error;
}

/* designated initializer */
- (id)initWithImageDescriptor:(AClass *)inImageDescriptor callback:(NSObject<MONImageRenderCallbackProtocol>*)inCallback;

@end

@implementation MONOperation

- (id)initWithImageDescriptor:(AClass *)inImageDescriptor callback:(NSObject<MONImageRenderCallbackProtocol>*)inCallback
{
    self = [super init];
    assert(self);
    if (0 != self) {

        assert(inImageDescriptor);
        imageDescriptor = [inImageDescriptor copy];

        assert(inCallback);
        callback = [inCallback retain];

        downloadSucceeded = 0;
        error = 0;

        if (0 == imageDescriptor || 0 == callback) {
            [self release];
            return 0;
        }
    }
    return self;
}

- (void)dealloc
{
    [imageDescriptor release], imageDescriptor = 0;
    [callback release], callback = 0;
    [error release], error = 0;

    [super dealloc];
}

/**
 @return an newly rendered NSImage, created based on self.imageDescriptor
 will set self.downloadSucceeded and self.error appropriately
 */
- (NSImage *)newImageFromImageDescriptor
{
    NSImage * result = 0;
    /* ... */
    return result;
}

- (void)main
{
    NSAutoreleasePool * pool = [NSAutoreleasePool new];

    NSImage * image = [self newImageFromImageDescriptor];

    if (downloadSucceeded) {
        assert(image);
        assert(0 == error);
        [callback imageRenderOperationSucceeded:imageDescriptor image:image];
        [image release], image = 0;
    }
    else {
        assert(0 == image);
        assert(error);
        [callback imageRenderOperationFailed:imageDescriptor withError:error];
    }

    [pool release], pool = 0;
}

@end