-allKeys on background thread results in error: __NSDictionaryM was mutated while being enumerated

Question

I've come across an interesting issue using mutable dictionaries on background threads.

Currently, I am downloading data in chunks on one thread, adding it to a data set, and processing it on another background thread. The overall design works for the most part aside from one issue: On occasion, a function call to an inner dictionary within the main data set causes the following crash:

*** Collection <__NSDictionaryM: 0x13000a190> was mutated while being enumerated.

I know this is a fairly common crash to have, but the strange part is that it's not crashing in a loop on this collection. Instead, the exception breakpoint in Xcode is stopping on the following line:

NSArray *tempKeys = [temp allKeys];

This leads me to believe that one thread is adding items to this collection while the NSMutableDictionary's internal function call to -allKeys is enumerating over the keys in order to return the array on another thread.

My question is: Is this what's happening? If so, what would be the best way to avoid this?

Here's the gist of what I'm doing:

dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^(void) {

    for (NSString *key in [[queue allKeys] reverseObjectEnumerator]) { //To prevent crashes

        NEXActivityMap *temp = queue[key];
        NSArray *tempKeys = [temp allKeys]; //<= CRASHES HERE
        if (tempKeys.count > 0) {
            //Do other stuff
        }
    }
});

Answer 1

You can use @synchronize. And it will work. But this is mixing up two different ideas:

• Threads have been around for many years. A new thread opens a new control flow. Code in different threads are running potentially concurrently causing conflicts as you had. To prevent this conflicts you have to use locks like @synchronized do.

• GCD is the more modern concept. GCD runs "on top of threads" that means, it uses threads, but this is transparent for you. You do not have to care about this. Code running in different queues are running potentially concurrently causing conflicts. To prevent this conflicts you have to use one queue for shared resources.

You are already using GCD, what is a good idea:

dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^(void) {

The same code with threads would look like this:

[[NSThread mainThread] performSelector:…];

So, using GCD, you should use GCD to prevent the conflicts. What you are doing is to use GCD wrongly and then "repair" that with locks.

Simply put all accesses to the shared resource (in your case the mutable dictionary referred by temp) into on serial queue.

1. Create a queue at the beginning for the accesses. This is a one-timer.

You can use one of the existing queues as you do in your code, but you have to use a serial one! But this potentially leads to long queues with waiting tasks (in your example blocks). Different tasks in a serial queue are executed one after each other, even there are cpu cores idle. So it is no good idea to put too many tasks into one queue. Create a queue for any shared resource or "subsystem":

dispatch_queue_t tempQueue;
tempQueue = dispatch_queue_create("tempQueue", NULL);

2. When code wants to access the mutable dictionary, put it in a queue:

It looks like this:

dispatch_sync( tempQueue, // or async, if it is possible
^{
  [tempQueue setObject:… forKey:…]; // Or what you want to do.
}

You have to put every code accessing the shared resource in the queue as you have to put every code accessing the shared resource inn locks when using threads.

Answer 2

From Apple documentation "Thread safety summary":

Mutable objects are generally not thread-safe. To use mutable objects in a threaded application, the application must synchronize access to them using locks. (For more information, see Atomic Operations). In general, the collection classes (for example, NSMutableArray, NSMutableDictionary) are not thread-safe when mutations are concerned. That is, if one or more threads are changing the same array, problems can occur. You must lock around spots where reads and writes occur to assure thread safety.

In your case, following scenario happens. From one thread, you add elements into dictionary. In another thread, you accessing allKeys method. While this methods copies all keys into array, other methods adds new key. This causes exception.

To avoid that, you have several options.

Because you are using dispatch queues, preferred way is to put all code, that access same mutable dictionary instance, into private serial dispatch queue.

Second option is passing immutable dictionary copy to other thread. In this case, no matter what happen in first thread with original dictionary, data still will be consistent. Note that you will probably need deep copy, cause you use dictionary/arrays hierarchy.

Alternatively you can wrap all points, where you access collections, with locks. Using @synchronized also implicitly create recursive lock for you.

Answer 3

How about wrapping where you get the keys AND where you set the keys, with @synchronize?

Example:

- (void)myMethod:(id)anObj
{
    @synchronized(anObj)
    {
        // Everything between the braces is protected by the @synchronized directive.
    }
}