Collection was mutated while being enumerated- archiving data and writing to file with NSCoder

Question

In my app I am periodically writing a set of dynamic data to file. The data object gets updated about every second. Occasionally I get a "Collection was mutated while being mutated" exception on one of the lines in my encodeWithCoder: method. Each object is encoded like this:

[aCoder encodeObject:self.speeds forKey:@"speeds"];

Where self.speeds is an NSMutableArray. I assume the problem is that the data is being updated while it is being encoded. I tried using @synchronize in the encoding and saving blocks and I also tried making the property atomic as opposed to nonatomic, but neither worked. The saving is happening in the background. Any ideas of how to save this data in the background while it is being updated? I feel like making a copy and then saving the copy would work, but wouldn't the same problem arise? Thanks!

---

Edit 1:

The idea in the app is that I open a map view, which periodically updates a singleton class with contains an array of data objects, with each data object being a user's map info. In each data object, the user's locations, speeds, altitudes, distance, etc. Every three times the location manager updates the user's location, I update the current data object (the 'live' data object which was just created to track this trip- there can only be one 'live' data object at any time) with the new information.

I would like to write the entire singleton to a file every x minutes, but sometimes the writing and the update happen at the same time and I get this error (or at least that is what I assume is causing this crash). Is the problem here with my code or my design pattern?

---

This is the encoding method in my custom class:

- (void)encodeWithCoder:(NSCoder*)aCoder {
    @synchronized([SingletonDataController sharedSingleton]) {
        [aCoder encodeObject:[[lineLats copy] autorelease] forKey:@"lineLats"];
        [aCoder encodeObject:[[lineLongs copy] autorelease] forKey:@"lineLongs"];
        [aCoder encodeObject:[[horizontalAccuracies copy] autorelease] forKey:@"horAcc"];
        [aCoder encodeObject:[[verticalAccuracies copy] autorelease] forKey:@"vertAcc"];
        [aCoder encodeObject:[[speeds copy] autorelease] forKey:@"speeds"];
        [aCoder encodeObject:[[overlayColors copy] autorelease] forKey:@"colors"];
        [aCoder encodeObject:[[annotationLats copy] autorelease] forKey:@"annLats"];
        [aCoder encodeObject:[[annotationLongs copy] autorelease] forKey:@"annLongs"];
        [aCoder encodeObject:[[locationManagerStartDate copy] autorelease] forKey:@"startDate"];
        [aCoder encodeObject:[[locationManagerStartDateString copy] autorelease] forKey:@"locStartDateString"];
        [aCoder encodeObject:[[mapTitleString copy] autorelease] forKey:@"title"];
        [aCoder encodeObject:[[shortDateStringBackupCopy copy] autorelease] forKey:@"backupString"];

        [aCoder encodeFloat:pathDistance forKey:@"pathDistance"];
        [aCoder encodeFloat:linearDistance forKey:@"linearDistance"];
        [aCoder encodeFloat:altitudeChange forKey:@"altitudeChange"];
        [aCoder encodeFloat:averageSpeedWithFilter forKey:@"avWithFilter"];
        [aCoder encodeFloat:averageSpeedWithoutFilter forKey:@"avWithoutFilter"];

        [aCoder encodeInt:totalTripTimeInSeconds forKey:@"totalTimeInSecs"];
    }
}

This is the update method (there is more code in the method and other methods called in the update method, but I'm omitting everything that doesn't reference the 'live' dataObject object; the one being updated):

- (void)locationManager:(CLLocationManager*)manager didUpdateToLocation:(CLLocation*)newLocation fromLocation:(CLLocation*)oldLocation {
    @synchronized([SingletonDataController sharedSingleton]) {
        //create temporary location for last logged location
        CLLocation* lastLocation;
        if([dataObject.lineLats lastObject] && [dataObject.lineLongs lastObject]) {
            lastLocation = [[CLLocation alloc] initWithLatitude:[[dataObject.lineLats lastObject] floatValue] longitude:[[dataObject.lineLongs lastObject] floatValue]];
        } else {
            lastLocation = [oldLocation retain];
        }

        //.....

        //periodically add horizontal/vertical accuracy
        if(iterations > 0 && iterations % 4 == 0) {
            [dataObject.horizontalAccuracies addObject:[NSNumber numberWithFloat:[newLocation horizontalAccuracy]]];
            [dataObject.verticalAccuracies addObject:[NSNumber numberWithFloat:[newLocation verticalAccuracy]]];
        }

        //.....

        //accumulate some speed data
        if(iterations % 2 == 0) {
            NSNumber* speedNum = [[NSNumber alloc] initWithFloat:[newLocation speed]];
            [dataObject.speeds addObject:speedNum];
            [speedNum release];
        }

        //.....

        //add latitude and longitude
        NSNumber* lat = [[NSNumber alloc] initWithFloat:[newLocation coordinate].latitude];
        NSNumber* lon = [[NSNumber alloc] initWithFloat:[newLocation coordinate].longitude];
        if(fabs([lat floatValue]) > .0001 && fabs([lon floatValue]) > .0001) {
            [dataObject.lineLats addObject:lat];
            [dataObject.lineLongs addObject:lon];
        }

        if(iterations % 60 == 0) {
            [[SingletonDataController sharedSingleton] synchronize];
        }
    }
}

And finally the synchronize method in the SingletonDataController class (updated so that now the synchronization occurs within the asynchronous block as per Tommy's answer):

dispatch_async(self.backgroundQueue, ^{
    @synchronized([SingletonDataController sharedSingleton]) {
        NSLog(@"sync");
        NSData* singletonData = [NSKeyedArchiver archivedDataWithRootObject:
            [SingletonDataController sharedSingleton]];

        if(!singletonData) {
            return;
        }

        NSString* filePath = [SingletonDataController getDataFilePath];
        [singletonData writeToFile:filePath atomically:YES];
    }
});

where backgroundQueue is created like this:

[sharedSingleton setBackgroundQueue:dispatch_queue_create("com.xxxx.xx", NULL)];

I can post more code if needed, but those seem to be the important parts.

Basset

I think the copy might work. Is it simple to try? – Anabas

Yes. The crash occurs pretty rarely, so it's hard to say whether the problem has been solved or not without pretty rigorous testing. But it's worth a try. I have moved the saving to the main thread and I believe I am still getting the same crash, though the iOS diagnostics report is a bit ambiguous. – Basset

@synchronize should be a possible solution, can you show your code which uses that? – Morgun

Answer 1

You perform a dispatch_async within one of your @synchronizes. The stuff in there isn't subject to the implicit lock built into the synchronise; all that happens it that you get the lock, dispatch the block and then release the lock. So the block can easily happen outside of the lock (and, indeed, you would expect that usually it will).

To stick with the synchronisation path, you want to @synchronize within the block rather than outside it. However you might try to come up with a less forceful approach, such as performing all your updates on a single serial dispatch queue and allowing them to push relevant new values onto the main queue.

Robey

Can you explain this a little further? I'm relatively new to queueing and GCD, and I'm trying not only to make it work but to understand it. What do you mean by performing all your updates on a single serial dispatch queue and allowing them to push relevant new values onto the main queue? – Basset

Assuming the first paragraph was fine — you ensure the block is dispatched while no-one else is modifying the data but have no safeguards on when it is performed — the alternative would be a serial dispatch queue on which all data updates are performed by your model object. I'm confused about your overall app architecture, but you'd have a CoreLocation agent in any runloop that dispatched to the model's queue; the model then does appropriate calculations and pushes new results to the controller on the main queue. The controller uses those results unless and until it receives new ones. – Robey

Serial dispatch queues can be read about here... developer.apple.com/library/ios/documentation/General/… – Artemisia

Answer 2

If you worry that the serialization takes long enough to affect the next serialization, copy the object, then use dispatch_async to serialize it. That way, the serialization will take place in an async queue.

However, maybe you want to rethink this approach entirely. Isn't Core Data an option? With it, you could only update the values that have actually changed, and I'm pretty sure it can handle your locking problems.

Edit Sorry I misread your initial post. If you don't save too often, you might want to consider using locks. See https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Multithreading/ThreadSafety/ThreadSafety.html

But only do that if you don't serialize too often, as this will significantly decrease your performance.

So, lock the object, copy it, unlock the object, serialize copy async.

Revanche

Alright. I am only serializing once every couple of minutes, so I don't think performance is too much of a concern. I'll take a look at locks. The thing is, is that the issue here? Serialization only takes a couple of seconds, and I am getting this error even when I only serialize once every minute or so. – Basset

Eh, if that's the case, why don't you just stop your updates while serialization is in progress? I mean, you update every second anyways, so that wouldn't matter. if(!serializationInProgress) { update() } – Revanche

So are you confirming that the reason for this crash is serialization while the data are being updated? Could it be something else entirely? – Basset

Answer 3

Yes, serializing a copy of the array instead of the mutable array will ensure that the array doesn't change while it's being saved, but you're just shifting the problem: you still have the case where the array could change on one thread while it's being copied on the other. You can put @synchronize blocks around both the copy and the array mutations (just like you said you were doing with the save/update.. that should have worked—were you using the same object for the @synchronize parameter? @synchronize(self) is a handy way to do it).

Another way to synchronize the copy operation is to use dispatch_sync() to make the copy on the main thread:

__block NSArray* listCopy;

dispatch_sync(dispatch_get_main_queue(), ^{ listCopy = [self.speeds copy]; });

[aCoder encodeObject:listCopy forKey:@"speeds"];
[listCopy release];

This is a bit more coarse-grained—it can't make the copy until the main thread is clear, whereas the @synchronized copy can run as soon as the main thread is out of its @synchronize block—but it has the advantage that you only have to put this code in the save thread, and not worry about where you might be changing the array in the main thread.

Edit: just saw the other note about using NSLock. Using @synchronize is pretty much the same thing as using an NSLock (here's a good SO post about it), but you don't have to worry about managing a lock object. Again, @synchronize should have worked for you, and it's really handy as long as you don't have dozens of different places you have to sync.

Linares

The reason I mentioned locks was because I was thinking he could use NSDistributedLocks, which would allow him to just pause updating until serialization/copying is complete. But yes, if @synchronize is usable, use it!^^ – Revanche

So why doesn't the same issue come up when making the copy of the array for serialization? – Basset

Well, copying should be much faster than encoding, therefore the probability of your problem happening should be really low. Doesn't make the app safe though, you should still try to @synchronize the copy-process – Revanche