Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue with AFMMRecordResponseSerializer threading #88

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix issue with AFMMRecordResponseSerializer threading #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c88bc6b
Fix Core Data threading problem with AFMMRecordResponseSerializer. T…
Jul 25, 2014
75dbe7a
Merge branch 'upstream' of https://github.com/jonbrooks/MMRecord into…
Jul 28, 2014
7eccced
Improved error handling for background context save
Jul 28, 2014
405931b
Move primaryKey injection earlier so it can successfully find/merge d…
Aug 18, 2014
2036559
Merge branch 'primaryKeyInjectionBugfix' of https://github.com/jonbro…
Aug 22, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions Source/AFMMRecordResponseSerializer/AFMMRecordResponseSerializer.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,23 @@ - (NSManagedObjectContext *)backgroundContext {
}

- (NSArray *)recordsFromMMRecordResponse:(MMRecordResponse *)recordResponse
backgroundContext:(NSManagedObjectContext *)backgroundContext {
__block NSMutableArray *objectIDs = [NSMutableArray array];
backgroundContext:(NSManagedObjectContext *)backgroundContext
options:(MMRecordOptions *)options {
NSMutableArray *objectIDs = [NSMutableArray array];

[backgroundContext performBlockAndWait:^{
NSError *error;
NSArray *records = [recordResponse records];
if (![backgroundContext save:&error]) {
NSDictionary *parameters = nil;
if (error.localizedDescription != nil) {
parameters = [options.debugger parametersWithKeys:@[MMRecordDebuggerParameterErrorDescription]
values:@[error.localizedDescription]];
}
[options.debugger handleErrorCode:MMRecordErrorCodeCoreDataSaveError
withParameters:parameters];
return;
}

for (MMRecord *record in records) {
[objectIDs addObject:[record objectID]];
Expand All @@ -120,9 +132,11 @@ - (NSArray *)recordsFromMMRecordResponse:(MMRecordResponse *)recordResponse

NSMutableArray *records = [NSMutableArray array];

for (NSManagedObjectID *objectID in objectIDs) {
[records addObject:[self.context objectWithID:objectID]];
}
[self.context performBlockAndWait:^{
for (NSManagedObjectID *objectID in objectIDs) {
[records addObject:[self.context objectWithID:objectID]];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}];

return records;
}
Expand Down Expand Up @@ -186,11 +200,12 @@ - (id)responseObjectForResponse:(NSURLResponse *)response

MMRecordResponse *recordResponse = [MMRecordResponse responseFromResponseObjectArray:responseArray
initialEntity:initialEntity
context:self.context
context:backgroundContext
options:options];

NSArray *records = [self recordsFromMMRecordResponse:recordResponse
backgroundContext:backgroundContext];
backgroundContext:backgroundContext
options:options];

*error = [debugger primaryError];

Expand Down
3 changes: 2 additions & 1 deletion Source/MMRecord/MMRecordProtoRecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@
*/
+ (MMRecordProtoRecord *)protoRecordWithDictionary:(NSDictionary *)dictionary
entity:(NSEntityDescription *)entity
representation:(MMRecordRepresentation *)representation;
representation:(MMRecordRepresentation *)representation
primaryKeyValue:(id)primaryKeyValue;


///---------------------------
Expand Down
5 changes: 3 additions & 2 deletions Source/MMRecord/MMRecordProtoRecord.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,13 @@ @implementation MMRecordProtoRecord

+ (MMRecordProtoRecord *)protoRecordWithDictionary:(NSDictionary *)dictionary
entity:(NSEntityDescription *)entity
representation:(MMRecordRepresentation *)representation {
representation:(MMRecordRepresentation *)representation
primaryKeyValue:(id)primaryKeyValue {
NSParameterAssert([NSClassFromString([entity managedObjectClassName]) isSubclassOfClass:[MMRecord class]]);
MMRecordProtoRecord *protoRecord = [[MMRecordProtoRecord alloc] init];
protoRecord.dictionary = dictionary;
protoRecord.entity = entity;
protoRecord.primaryKeyValue = [representation primaryKeyValueFromDictionary:dictionary];
protoRecord.primaryKeyValue = primaryKeyValue;
protoRecord.relationshipProtosDictionary = [NSMutableDictionary dictionary];
protoRecord.relationshipDescriptionsDictionary = [NSMutableDictionary dictionary];
protoRecord.hasRelationshipPrimarykey = [representation hasRelationshipPrimaryKey];
Expand Down
39 changes: 18 additions & 21 deletions Source/MMRecord/MMRecordResponse.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,35 +223,32 @@ - (MMRecordProtoRecord *)protoRecordWithRecordResponseObject:(id)recordResponseO
}

id primaryValue = [representation primaryKeyValueFromDictionary:recordResponseObject];
MMRecordProtoRecord *proto = [recordResponseGroup protoRecordForPrimaryKeyValue:primaryValue];

if (proto == nil) {
proto = [MMRecordProtoRecord protoRecordWithDictionary:recordResponseObject
entity:entity
representation:representation];


if (proto.hasRelationshipPrimarykey == NO) {
if (proto.primaryKeyValue == nil) {
if (self.options.entityPrimaryKeyInjectionBlock != nil) {
proto.primaryKeyValue = self.options.entityPrimaryKeyInjectionBlock(proto.entity,
proto.dictionary,
parentProtoRecord);
}
}

if (proto.primaryKeyValue == nil) {
if (primaryValue == nil) {
if (self.options.entityPrimaryKeyInjectionBlock != nil) {
primaryValue = self.options.entityPrimaryKeyInjectionBlock(entity,
recordResponseObject,
parentProtoRecord);
if (primaryValue == nil) {
MMRecordDebugger *debugger = self.options.debugger;
NSString *errorDescription = [NSString stringWithFormat:@"Creating proto record with no primary key value. \"%@\"", proto];
NSString *errorDescription = [NSString stringWithFormat:@"No primary key value for response object. \"%@\"", recordResponseObject];
NSDictionary *parameters = [debugger parametersWithKeys:@[MMRecordDebuggerParameterRecordClassName,
MMRecordDebuggerParameterErrorDescription,
MMRecordDebuggerParameterEntityDescription]
values:@[proto.entity.managedObjectClassName,
values:@[entity.managedObjectClassName,
errorDescription,
proto.entity]];
entity]];
[debugger handleErrorCode:MMRecordErrorCodeMissingRecordPrimaryKey withParameters:parameters];
}
}
}
MMRecordProtoRecord *proto = [recordResponseGroup protoRecordForPrimaryKeyValue:primaryValue];

if (proto == nil) {
proto = [MMRecordProtoRecord protoRecordWithDictionary:recordResponseObject
entity:entity
representation:representation
primaryKeyValue:primaryValue];

} else {
[representation.marshalerClass mergeDuplicateRecordResponseObjectDictionary:recordResponseObject
withExistingProtoRecord:proto];
Expand Down