53

I've been trying to fix this crash for almost a week. The application crashes without any exception or stack-trace. The application does not crash in any way while running through instruments in zombie mode.

I have a method that gets called on a different thread. The solution that fixed the crash was replacing

[self.mutableArray removeAllObjects];

with

dispatch_async(dispatch_get_main_queue(), ^{
    [self.searchResult removeAllObjects];
});

I thought it might be a timing issue, so I tried to synchronize it, but it still crashed:

@synchronized(self)
{
    [self.searchResult removeAllObjects];
}

Here is the code

- (void)populateItems
{
   // Cancel if already exists  
   [self.searchThread cancel];

   self.searchThread = [[NSThread alloc] initWithTarget:self
                                               selector:@selector(populateItemsinBackground)
                                                 object:nil];

    [self.searchThread start];
}


- (void)populateItemsinBackground
{
    @autoreleasepool
    {
        if ([[NSThread currentThread] isCancelled])
            [NSThread exit];

        [self.mutableArray removeAllObjects];

        // Populate data here into mutable array

        for (loop here)
        {
            if ([[NSThread currentThread] isCancelled])
                [NSThread exit];

            // Add items to mutableArray
        }
    }
}

Is this problem with NSMutableArray not being thread-safe?

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
aryaxt
  • 76,198
  • 92
  • 293
  • 442
  • 3
    One `@synchronized` block by itself wouldn't do anything. You need a lock (which is what `@sychronized` provides) around _every_ access to the array. – jscs Aug 23 '12 at 18:43
  • 1
    List of thread safe and unsafe classes https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html – Jageen Jan 09 '16 at 19:47
  • Why @ autoreleasepool is here ? can anybody clarify me ? – MuraliMohan Feb 24 '17 at 12:50
  • @MuraliMohan"If you are making Cocoa calls outside of the Application Kit’s main thread—for example if you create a Foundation-only application or if you detach a thread—you need to create your own autorelease pool." https://developer.apple.com/reference/foundation/nsautoreleasepool – aryaxt Feb 24 '17 at 16:47
  • 1
    In my view, the best way to handle the readers/writers access problems is to use the dispatch_barriers provided by GCD (Grand Central Dispatch). – XLE_22 Mar 19 '18 at 14:24

7 Answers7

90

No.

It is not thread safe and if you need to modify your mutable array from another thread you should use NSLock to ensure everything goes as planned:

NSLock *arrayLock = [[NSLock alloc] init];

[...] 

[arrayLock lock]; // NSMutableArray isn't thread-safe
[myMutableArray addObject:@"something"];
[myMutableArray removeObjectAtIndex:5];
[arrayLock unlock];
Daniel
  • 23,129
  • 12
  • 109
  • 154
  • 33
    You need to do that **and** protect every *read* with the same lock as well. – Adam Rosenfield Aug 23 '12 at 18:55
  • 1
    Is this necessary if the @property is declared without the `nonatomic` specifier? – Mark Adams Aug 23 '12 at 19:01
  • It all depends on the implementation of the getter/setter or not due to @synthesize, check out this post: http://stackoverflow.com/a/589392/662605 – Daniel Aug 23 '12 at 19:06
  • I replaced dispatch_async with a lock, and it still crashed – aryaxt Aug 23 '12 at 20:25
  • 1
    How did you do that? You surrounded the removeAllObjects by the lock? Where is your lock initialised ? – Daniel Aug 23 '12 at 20:38
  • @Daniel I surrounded it with a lock, I initialized the lock right before calling lock. Does it matter where I initialize the lock method? Do I still need to use dispatch along with the lock? – aryaxt Aug 23 '12 at 20:54
  • 2
    yes you should initialise the lock from the thread the object you want locking is initialised. Try putting the lock next to where you initialise your array, and make it an ivar – Daniel Aug 23 '12 at 21:23
  • Firstly, thanks for that! For read actions you can use [mutableArray copy]. – digipeople Oct 04 '13 at 18:59
  • There are a lot of possibilities to make a lock besides NSLock. Some of them are more efficient. See this [post](http://perpendiculo.us/2009/09/synchronized-nslock-pthread-osspinlock-showdown-done-right/) – malex Jan 05 '14 at 23:04
39

As others already said, NSMutableArray is not thread safe. In case anyone want to achieve more than removeAllObject in a thread-safe environment, I will give another solution using GCD besides the one using lock. What you have to do is to synchronize the read/update(replace/remove) actions.

First get the global concurrent queue:

dispatch_queue_t concurrent_queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);

For read:

- (id)objectAtIndex:(NSUInteger)index {
    __block id obj;
    dispatch_sync(self.concurrent_queue, ^{
        obj = [self.searchResult objectAtIndex:index];
    });
    return obj;
}

For insert:

- (void)insertObject:(id)obj atIndex:(NSUInteger)index {
    dispatch_barrier_async(self.concurrent_queue, ^{
        [self.searchResult insertObject:obj atIndex:index];
    });
}

From Apple Doc about dispatch_barrier_async:

When the barrier block reaches the front of a private concurrent queue, it is not executed immediately. Instead, the queue waits until its currently executing blocks finish executing. At that point, the barrier block executes by itself. Any blocks submitted after the barrier block are not executed until the barrier block completes.

Similar for remove:

- (void)removeObjectAtIndex:(NSUInteger)index {
    dispatch_barrier_async(self.concurrent_queue, ^{
        [self.searchResult removeObjectAtIndex:index];
    });
}

EDIT: Actually I found another simpler way today to synchronize access to a resource by using a serial queue provided by GCD.

From Apple Doc Concurrency Programming Guide > Dispatch Queues:

Serial queues are useful when you want your tasks to execute in a specific order. A serial queue executes only one task at a time and always pulls tasks from the head of the queue. You might use a serial queue instead of a lock to protect a shared resource or mutable data structure. Unlike a lock, a serial queue ensures that tasks are executed in a predictable order. And as long as you submit your tasks to a serial queue asynchronously, the queue can never deadlock.

Create your serial queue:

dispatch_queue_t myQueue = dispatch_queue_create("com.example.MyQueue", NULL);

Dispatch tasks async to the serial queue:

dispatch_async(myQueue, ^{
    obj = [self.searchResult objectAtIndex:index];
});

dispatch_async(myQueue, ^{
    [self.searchResult removeObjectAtIndex:index];
});

Hope it helps!

Jingjie Zhan
  • 985
  • 8
  • 12
  • 1
    Using gcd might cause problems. What if the value is expected to be removed/added to an error before the end of the runloop? – aryaxt Feb 18 '14 at 02:25
  • Just perform _all_ operations on the same serial queue. – gnasher729 Feb 18 '14 at 23:55
  • Serial queue is of course solution, but reading from collections of any kind **is** thread-safe. Parallel reading and barrier modifications are perfectly fine. – Tricertops May 02 '14 at 20:10
  • 14
    Jingjie, your original answer, using barrier, is good answer, as this is a far more efficient mechanism than the lock approach of the accepted answer. It's also better than serial queue. Future readers should see "Reader-Writer" discussion in [WWDC 2012 video, Asynchronous Design Patterns with Blocks, GCD, and XPC](https://developer.apple.com/videos/wwdc/2012/?id=712). Note, though, one should never use barriers on global queues (note reference to "private" queue). Use custom concurrent queues, e.g. `concurrent_queue = dispatch_queue_create("com.domain.queuename", DISPATCH_QUEUE_CONCURRENT);`. – Rob Dec 18 '14 at 14:05
  • 4
    Emphasizing what Rob said, not only should you not use dispatch_barrier on global queues, it won't work properly if you do. From Apple: `The queue you specify should be a concurrent queue that you create yourself using the dispatch_queue_create function. If the queue you pass to this function is a serial queue or one of the global concurrent queues, this function behaves like the dispatch_async function.` – scosman May 24 '16 at 17:40
  • I am using first solution with semaphore. I got deadlock once without semaphore. semaphore count is 20. – Parag Bafna Nov 08 '17 at 04:16
21

As well as NSLock can also use @synchronized(condition-object) you just have to make sure every access of the array is wrapped in a @synchronized with the same object acting as the condition-object , if you only want to modify the contents of the same array instance then you can use the array itself as the condition-object, other wise you will have to use something else you know will not go away, the parent object, i.e self, is a good choice because it will always be the same one for the same array.

atomic in @property attributes will only make setting the array thread safe not modifying the contents, i.e. self.mutableArray = ... is thread safe but [self.mutableArray removeObject:] is not.

Sangram Shivankar
  • 3,535
  • 3
  • 26
  • 38
Nathan Day
  • 5,981
  • 2
  • 24
  • 40
14
__weak typeof(self)weakSelf = self;

 @synchronized (weakSelf.mutableArray) {
     [weakSelf.mutableArray removeAllObjects];
 }
rustyMagnet
  • 3,479
  • 1
  • 31
  • 41
sash
  • 8,423
  • 5
  • 63
  • 74
  • Just for the record, your synchronized sentence does not make any sense itself. It does make sense if you wrap it with read write operations to avoid race conditions, but wrapping removeAllObjects method does not add any value. It should be done in a higher level use case. – GoRoS Apr 15 '16 at 07:08
  • @GoRoS not sure why it doesn't make sense for you because it creates mutex lock and prevents different threads from accessing this array. – sash Apr 15 '16 at 08:31
  • here you have a brief explanation of what I meant: http://stackoverflow.com/a/21139721/812598 – GoRoS Apr 15 '16 at 08:34
  • @GoRos synchronized only prevents accessing the array from multiple threads at the same time, it doesn’t prevent from logical errors. Basically if you remove all objects on one thread and try to access any object on a different thread you will still have a crash. Synchronized will just prevent that removing and accessing objects doesn’t happen at the exact same time. You can do may actions inside the synchronized block. Read the article. – sash Apr 15 '16 at 09:15
  • Sorry but I don't agree at all. If you remove all objects with the code of the answer and access to an element from another, it will still crash. It should be the other thread's code block with a higher business level use case the responsible to lock the mutable array, perform read/write operations and unlock it. Nothing between those operations will be done. In your link it is the same, the synchronized block wrapps a read and write operations which makes sense. Look at the previous comment link to find out why I'm saying it's wrong. – GoRoS Apr 15 '16 at 09:39
  • Yes, it will crash, because it is logical error. You should check the length of the array before you do anything. This example only makes sure that while removeAllObjects happens, no other thread access this array. Sorry, I can’t explain to you any better :) – sash Apr 15 '16 at 09:57
5

Since serial queues were mentioned: With a mutable array, just asking "is it thread safe" isn't enough. For example, making sure that removeAllObjects doesn't crash is all good and fine, but if another thread tries to process the array at the same time, it will either process the array before or after all elements are removed, and you really have to think what the behaviour should be.

Creating one class + object that is responsible for this array, creating a serial queue for it, and doing all operations through the class on that serial queue is the easiest way to get things right without making your brain hurt through synchronisation problems.

gnasher729
  • 51,477
  • 5
  • 75
  • 98
1

All the NSMutablexxx classes are not thread-safe. Operations including get,insert,remove,add and replace should be used with NSLock.This is a list of thread-safe and thread-unsafe classes given by apple: Thread Safety Summary

holybiner
  • 61
  • 2
0

Almost NSMutable classes object is not thread safe.

Hardeep Singh
  • 942
  • 8
  • 15