Fix issue with AFMMRecordResponseSerializer threading

[cnstoll]

I created a new pull request with some additional error handling using the built in debugger. @jonbrooks please take a look.

[cnstoll]

Fixes #85

[jkrall]

+1 just ran into this myself... this looks like a good change.

[jonbrooks]

Yes, this looks good to me too. The error handling is definitely an improvement over what I had.

[jkrall]

Actually, this code is throwing the following error for me now... any ideas why? (sorry, I'm a CoreData newbie, so still figuring out whats going on here)

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', 
reason: 'This NSPersistentStoreCoordinator has no persistent stores.  It cannot perform a save operation.'

[cnstoll]

That's code for "something is happening on the wrong thread." Need to take another look at this then.

On Jul 30, 2014, at 12:26 AM, Joshua Krall [email protected] wrote:

Actually, this code is throwing the following error for me now... any ideas why? (sorry, I'm a CoreData newbie, so still figuring out whats going on here)

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException',
reason: 'This NSPersistentStoreCoordinator has no persistent stores. It cannot perform a save operation.'
—
Reply to this email directly or view it on GitHub.

[jkrall]

Actually I think this is my own fault... I have some code in my DataManager that is removing the persistence store on logout (not sure why, I cargo-culted it)... and then it obviously isn't being recreated when I need it again on login.

So the error is correct, there's no persistence store b/c I removed it. Not thread-related at all, I think.

[jkrall]

Hah... so turns out I cargo-culted it from here: https://github.com/mutualmobile/MMRecord/blob/master/Examples/MMRecordPerformance/MMRecordAppDotNet/MMDataManager.m

Any idea if this code is good? (at the very least, it seems like the singleton pattern I've seen elsewhere is to wrap the sharedInstance allocation in a dispatch_once block, tho that isn't related)

Removing the call to cleanupPersistantStoresWithCoordinator solved my problem. Maybe resetting the ManagedObjectContext is good enough?

[jkrall]

Possible issue here... you are storing the objectID's above in the background context, and calling save to persist these objects in the backgroundContext. However, then here you query the main self.context for these objectIDs... and in my app I'm getting back the previous copy of an object. (if I have a User object already in the store, and then a response comes back with the same ID but different data payload, it seems to be persisted in the store, but self.context has the old copy still, which is what gets returned)

I tried changed the above code to just put the records directly in the NSMutableArray are return it... rather than storing the objectIDs and then re-querying for them... but this didn't work. (I guess you had a good reason for doing it this way)

Adding this refreshObject line did fix my issue however:

            NSManagedObject *record = [self.context objectWithID:objectID];
            [self.context refreshObject:record mergeChanges:NO];
            [records addObject:record];

[jonbrooks]

Good catch. I think I misunderstood the behavior of objectWithID:. I think this change should go in.

[jkrall]

Can you comment on why you are constructing the array of IDs and then re-querying the self.context by objectID to build the array of actual objects... rather than just building the mutable array of objects once? (i tried this and it didnt work, but I confess that I dont really get why)

[jonbrooks]

If I understand your question, you're asking why not just return the array of NSManagedObjects that we get from the backgroundContext? Ultimately, we want to return NSManagedObjects associated with the main context. The work here is all done on the backgroundContext (this is now the context associated with the recordResponse), and the returned objects are associated with that context. We must then use ObjectID as a way of translating those same objects to the main context. Does that answer your question?

[jkrall]

Yep, got it. That's what I thought, but I wasn't really aware that the object you get from the backgroundContext are different than those from the main context. (they obviously have different data in some places... but I need to read more about how the different contexts work)

Thanks!

[cnstoll]

So this is all very interesting. Josh, I think you're right that refreshObject should be called there for this to work as expected. Jon is also correct that the reason we're using objectWithID is to make sure objects are associated with the right context. Thats important because NSManagedObjects are tied to the context/thread they are associated with and accessing them outside of that can be dangerous. We are performing all the parsing work on a background context for performance reasons, but generally speaking when people use a library like this they want to access the objects on the main thread to populate their UI, so we migrate the changes over to the context they specify.

I'm trying to spend a bit more time thinking about this though, because there are a few complexity issues. For one thing, how people expect this to behave might depend on how they setup their Core Data stack. I don't want to go into a ton of detail, but sufficed it to say that contexts act very differently depending on if a context is a parent/child context versus a "root" context with no parent or children. Save only pushes changes up one level, from the child to the parent, or from the parent to the persistent store. Fetch, on the other hand, pulls all the way through to the persistent store.

Also, MMRecord.m itself is actually doing this step differently. It's actually using the class Core Data "mergeChangesFromNotification" method of merging changes between contexts. Its a bit of a hassle, but it doesn't have some of the weird issues we're dealing with here, so thats why it does it.

I'm considering some alternatives to this as well, such as moving the MMRecord.m implementation of context syncing up a level into the MMRecordResponse class. Or, we could go with this implementation which relies on refreshObject. The only downside there is that if the main context has unsaved changes to those objects, then those changes might be lost. This would be a nasty bug for someone to solve if they didn't expect MMRecord to be doing this. If I had to make a decision now, I'd go with the refreshObject option, but I'd love to hear what you guys think first. And I'll probably spend some more time this week thinking about it before we make a decision.

[jkrall]

Fascinating. +100 for teaching me about CoreData via this issue/PR :)

IMO, the MMRecord.m approach of observing merge events is cleaner and more "correct". It seems like this kind of thing needs to be done right/carefully or people are going to run into weird & unexpected bugs.

Looking at the implementation here: https://github.com/mutualmobile/MMRecord/blob/master/Source/MMRecord/MMRecord.m#L1003
... it really doesn't seem too bad. (I might be missing some other piece)

Agree that it would be cool to move this up to some shared place, but even replicating this logic in AFMMRecordResponseSerializer seems reasonable to me.

[jonbrooks]

Sorry - I was away and didn't have a chance to respond to this. I wasn't aware of the complexities around parent/child contexts either. And I'll second Josh's statement about learning about CoreData. I've learned a lot from both this issue and MMRecord's source.

From a code evolution standpoint, the refreshObject solution seems like a reasonable step forward. There may still be problems with more complicated CoreData setups, but at least it will prevent crashes in the basic cases. Some thought should to be given on whether to merge changes or not.

I don't have a clear picture of what the other solution you mention would look like - or what problems it may have - but on the face of it, it seems like that could be a better long term solution. But again, I don't see any reason not to move forward with the refreshObject one in the interim.

[mikekhristo]

Hey guys -

I'm running into similar issues with the latest MMRecord from cocoapods. Is this PR going to make it into the next release? Or is it abandoned? Seems like it's been sitting stale since August.

Thanks!

[cnstoll]

This will go in the next release. My apologies Mike that it's taken longer than I expected to get to it. The iOS 8 release cycle was fairly action packed. I do hope to get to this soon though. For now, feel free to patch in the commit manually in a branch if you need to.

Thanks,

• Conrad

[mikekhristo]

Certainly no apology necessary. Patched it last night and things seem to be so much better. Thanks for all your hard work!

[cnstoll]

👍 thanks!

[jonbrooks]

@cnstoll - I think I'm still encountering issues stemming from this. Last I remember, we agreed on a fix that added [self.context refreshObject:record mergeChanges:NO]; to the records returned in [AFMMRecordResponseSerializer recordsFromMMRecordResponse:backgroundContext:options:]. I'm finding this is not enough. These are only the top-level objects, but I'm finding other objects that may have been changed by the response will still be un-refreshed on the main context. Now, I'm understanding the value of the NSManagedObjectContextObjectsDidChangeNotification strategy. Thoughts?