MTLManagedObjectAdapter could delete existing object if there is validation error #223

Closed
kylef opened this Issue Jan 27, 2014 · 5 comments

Comments

Projects
None yet
3 participants
@kylef
Member

kylef commented Jan 27, 2014

Discovered while making #220. If validateForInsert returns NO and the object came from a uniquing predicate, then it could be deleted. I think it would be better if it didn't make any changes and shouldn't delete existing objects.

I'm not sure how this could clearly be done, hence why I'm making an issue instead of a pull request. Other than making a child-context and throwing it away on failure.

Here is the basic code-path for this issue, it first gets an object using the uniquingPredicate, validateForInsert then returns NO and then it deletes the object in deleteObject.

    if (uniquingPredicate != nil) {
        managedObject = performInContext(context, ^ id {
                        // ...
            fetchRequest.predicate = uniquingPredicate;
            fetchRequest.returnsObjectsAsFaults = NO;
            fetchRequest.fetchLimit = 1;

            NSArray *results = [context executeFetchRequest:fetchRequest error:&fetchRequestError];

                        // ...
            return results.mtl_firstObject;
        });
    }

    // ...

    if (managedObject != nil && ![managedObject validateForInsert:&tmpError]) {
        managedObject = performInContext(context, ^ id {
            [context deleteObject:managedObject];
            return nil;
        });
    }
@jspahrsummers

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 31, 2014

Owner

Hmm, good catch.

We could definitely use a child context, though some of the historical issues with those scare me off a bit. Any idea how many of those are still applicable today?

Owner

jspahrsummers commented Jan 31, 2014

Hmm, good catch.

We could definitely use a child context, though some of the historical issues with those scare me off a bit. Any idea how many of those are still applicable today?

@kylef

This comment has been minimized.

Show comment Hide comment
@kylef

kylef Jan 31, 2014

Member

@jspahrsummers Which issues are these?

Member

kylef commented Jan 31, 2014

@jspahrsummers Which issues are these?

@jspahrsummers

This comment has been minimized.

Show comment Hide comment
@kylef

This comment has been minimized.

Show comment Hide comment
@kylef

kylef Jan 31, 2014

Member

Basically, when you execute a fetch request in a child context, it blocks the parent context while performing the fetch. What does this mean? Well it means that you can’t really use nested contexts to solve a lot of concurrency issues.

It was never designed to solve this problem. It's known and documented how fetching though parent context's can block the parent. Apple instead suggest you use dual persistent store coordinators to make use of WAL (Write Ahead Locking) to do things concurrently. Using the child/parent context for this use is a bad idea and has he has shown, it could actually make your Core Data stack far slower since the more levels of context you add. Check out WWDC 2013/211 for more info.

I'm going to go ahead and make a pull-request for this (when I get a spare moment), we can have a play around and see how it works out. Ensuring there are appropriate tests so we know it isn't broken.

Member

kylef commented Jan 31, 2014

Basically, when you execute a fetch request in a child context, it blocks the parent context while performing the fetch. What does this mean? Well it means that you can’t really use nested contexts to solve a lot of concurrency issues.

It was never designed to solve this problem. It's known and documented how fetching though parent context's can block the parent. Apple instead suggest you use dual persistent store coordinators to make use of WAL (Write Ahead Locking) to do things concurrently. Using the child/parent context for this use is a bad idea and has he has shown, it could actually make your Core Data stack far slower since the more levels of context you add. Check out WWDC 2013/211 for more info.

I'm going to go ahead and make a pull-request for this (when I get a spare moment), we can have a play around and see how it works out. Ensuring there are appropriate tests so we know it isn't broken.

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Jul 26, 2015

Owner

Closing this one out for age ✌

Owner

robb commented Jul 26, 2015

Closing this one out for age ✌

@robb robb closed this Jul 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment