Thread-safety: is making a data structure immutable enough?

Question

I have a class that is accessed from different threads and which modifies the content of an array. I started with using a NSMutableArray but it was obviously not thread safe. Will it solve the thread safety problem to replace the NSMutableArray with a NSArray and make copies when needed?

For example:

@implementation MyClass {
    NSArray *_files;
}

- (void)removeFile:(NSString *)fileName {
    NSMutableArray *mutableFiles = [_files mutableCopy];
    [mutableFiles removeObject:fileName];
    _files = [mutableFiles copy];
}

instead of:

@implementation MyClass {
    NSMutableArray *_files;
}

- (void)removeFile:(NSString *)fileName {
    [_files removeObject:fileName];
}

Making copies is not so critical in my case since the array will stay pretty small and the remove operation won't be executed so often.

Answer 1

No, that is insufficient to ensure thread-safety. You have to use one of the various Synchronization techniques outlines in the Threading Programming Guide, (e.g. using locks, such as NSLock or @synchronized).

Or, often more efficient, you can use a serial queue to synchronize the object (see Eliminating Lock-Based Code section of the Migrating Away From Threads chapter of the Concurrency Programming Guide). While @synchronized is incredibly simple, I'd lean towards this latter approach of a dedicated serial queue to synchronize access:

// The private interface

@interface MyClass ()

@property (nonatomic, strong) NSMutableArray   *files;
@property (nonatomic, strong) dispatch_queue_t  fileQueue;

@end

// The implementation

@implementation MyClass

- (instancetype)init
{
    self = [super init];
    if (self) {
        _files = [[NSMutableArray alloc] init];
        _fileQueue = dispatch_queue_create("com.domain.app.files", DISPATCH_QUEUE_SERIAL);
    }
    return self;
}

- (void)removeFile:(NSString *)fileName
{
    dispatch_async(_fileQueue, ^{
        [_files removeObject:fileName];
    });
}

- (void)addFile:(NSString *)fileName
{
    dispatch_async(_fileQueue, ^{
        [_files addObject:fileName];
    });
}

@end

The key in thread-safety is to ensure that all interaction with the object in question is synchronized. Simply using an immutable object is insufficient. And just wrapping removeFile in a @synchronized block is also insufficient. You generally want to synchronize all interaction with the object in question. You generally cannot just return the object in question and let the caller start using it without synchronizing its interaction. So, I might provide a method that allows the caller to interact with this files array in a thread-safe manner:

/** Perform some task using the files array
 *
 * @param block This is the block to be performed with the `files` array.
 *
 * @note        This block does not run on the main thread, so if you are doing any
 *              UI interaction, make sure to dispatch that back to the main queue.
 */
- (void)performMutableFileTaskWithBlock:(void (^)(NSMutableArray *files))block
{
    dispatch_sync(_fileQueue, ^{
        block(_files);
    });
}

You can then call that like so:

[myClassObject performMutableFileTaskWithBlock:^(NSMutableArray *files) {
    // do whatever you want with the files array here
}];

Personally, it gives me the willies to let the caller do whatever it wants with my array (I'd rather see MyClass provide an interface for whatever manipulation is needed). But if I need a thread-safe interface for the caller to access the array, I might prefer to see a block method like so, that provides a block interface with a deep copy of the array:

/** Perform some task using the files array
 *
 * @param block This is the block to be performed with an immutable deep copy of `files` array.
 */
- (void)performFileTaskWithBlock:(void (^)(NSArray *files))block
{
    dispatch_sync(_fileQueue, ^{
        NSArray *filesDeepCopy = [[NSArray alloc] initWithArray:_files copyItems:YES]; // perform deep copy, albeit only a one-level deep copy
        block(filesDeepCopy);
    });
}

Going to your immutable question, the one thing you might do, though, is have a method that returns an immutable copy of the object in question, which you'll let the caller use as it sees fit, with an understanding that this represents the files array as a snapshot in time. (And, like above, you'd do a deep copy.)

/** Provide caller with a copy of the files array
 *
 * @return A deep copy of the files array.
 */
- (NSArray *)filesCopy
{
    NSArray __block *filesCopy;
    dispatch_async(_fileQueue, ^{
        filesCopy = [[NSArray alloc] initWithArray:_files copyItems:YES]; // perform deep copy
    });

    return filesCopy;
}

Clearly, though, this has limited practical usage. For example, where you're dealing with an array of file names, it might not be appropriate to return a immutable copy of that array if those file names correspond to actual physical files that might be manipulated by another thread. But in some cases, the above is a fine solution. It depends entirely upon the business rules of the model object in question.

Answer 2

No it won't, you need to use @synchronized in your method to prevent multiple calls to removeFile: from being executed in parallel.

Like this:

- (void)removeFile:(NSString *)fileName {
    @synchronized(self)
    {
        [_files removeObject:fileName];
    }
}

The reason it will not work with your code is that multiple threads calling removeFile: at the same time can cause this to occur:

NSMutableArray *mutableFiles1 = [_files mutableCopy]; // Thread 1
[mutableFiles1 removeObject:fileName1];
// Thread 1 is interrupted, Thread 2 is run
NSMutableArray *mutableFiles2 = [_files mutableCopy]; // Thread 2
[mutableFiles2 removeObject:fileName2];
_files = [mutableFiles2 copy];
// Thread 1 is continued
_files = [mutableFiles1 copy];

At which point _files still contains fileName2

It is a race condition so it might look okay and work 99% of the time, but it is not guaranteed to be correct.