magicalpanda / MagicalRecord

Super Awesome Easy Fetching for Core Data!
Other
10.8k stars 1.79k forks source link

Correct way to use MagicalRecord in a concurrent NSOperation #1070

Open maystroh opened 9 years ago

maystroh commented 9 years ago

With MR_contextForCurrentThread not being safe for Operations (and being deprecated), Im trying to ensure I understand the best pattern for series of read/writes in a concurrent operations. It's been advised to use saveWithBlock for storing new records, which provides a context for use. The Count and fetch methods can be given a context, but still use MR_contextForCurrentThread by default.

Is the safest pattern to obtain a context using [NSManagedObjectContext MR_context] at the start of the operation, and use it for all actions. The operation depends on some async work, but not long running. Then perform a MR_saveToPersistentStoreWithCompletion when the operation is finished?

So, I already implemented this approach and to make sure I have no problems I turned on the concurrency core data violation in XCode. But the results were not what I expected, the app stops when creating an entity in an operation using the private context of that operation which means I violated the concurrency core data rules. In this case, what will be the best solution to avoid such problems? I also tried to use [privateContext performblock^:(){ //code of private context }] and it worked since XCode didn't stop the app but that way I'm not using anymore the main functionalities of MagicalRecord so i might have to replace MagicalRecord by simple architecture of ManagedObjectContext (main and private ones). Advise please.

iv-mexx commented 9 years ago

I usually do it like this

[MagicalRecord saveWithBlock:^(NSManagedObjectContext *localContext) {
    ModelObject *beacon = [ModelObject MR_importFromObject:someJSON inContext:localContext];
    // Maybe do something else here...
}];

This does not trigger the multithreading assertion.

maystroh commented 9 years ago

I'm not asking about how we can save the changes.. my main question is how can we handle threading that do fetching/counting data now taking into account that MR_contextForCurrentThread is not safe anymore? @iv-mexx

Alydyn commented 9 years ago

@maystroh you create your context in the operations -start method:

self.localContext = [NSManagedObjectContext MR_context];`
[self.localContext performBlock:^{
    //do stuff, like counting or w/e
}];

Now technically the call to -performBlock probably isn't needed, but I always wrap my access in -performBlock now to prevent accidental improper access.

Keep in mind also that pretty much all of the MagicalRecord category fetches already wrap the fetch in -performBlockAndWait: so if you are just fetching all you should have to do is pass in a context.

maystroh commented 9 years ago

Sounds good as solution for concurrent operations. Should not have something in MagicalRecord that does this internally?

Alydyn commented 9 years ago

MagicalRecord explicitly does not handle concurrency for you. There are too many ways that a default concurrency implementation could go wrong. Could you close this issue if you believe you have a solution? Thank you!

casademora commented 9 years ago

MagicalRecord does the performBlock and performBlockAndWait calls for you under the covers, but right now, it’s only for save operations. Are there other operations you’re doing that we should think about adding support for?

Saul Mora saul@casademora.com mailto:saul@casademora.com

On Aug 28, 2015, at 6:50 AM, Aaron Abt notifications@github.com wrote:

MagicalRecord explicitly does not handle concurrency for you. There are too many ways that a default concurrency implementation could go wrong. Could you close this issue if you believe you have a solution? Thank you!

— Reply to this email directly or view it on GitHub https://github.com/magicalpanda/MagicalRecord/issues/1070#issuecomment-135779720.

maystroh commented 9 years ago

Considering the fact that MR_contextForCurrentThread is not being safe for Operations, the question is why you're just using the performBlock and performBlockAndWait calls for the save operations and not for read/fetch ones? isn't the same problem everywhere?

hardikdevios commented 9 years ago

:+1: @maystroh you are pointing out a very good point but genrally people are using a Default Context for Reading and fetching just to make them self feel safe because if we are doing any private context for reading and fetching then its our responsibility to make and merge those changes to default context if we change that object , thats why its limited to Save only.

maystroh commented 9 years ago
(NSArray *) MR_findAllWithPredicate:(NSPredicate *)searchTerm 
{
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
    return [self MR_findAllWithPredicate:searchTerm inContext:[NSManagedObjectContext MR_contextForCurrentThread]];
#pragma clang diagnostic pop
}

In this method, you get the context of the current thread and use it to fetch the data which might crash the app based on the fact [NSManagedObjectContext MR_contextForCurrentThread] is not safe for operations. So you already added new methods to handle the save operations without using [NSManagedObjectContext MR_contextForCurrentThread] but you keep using it in all fetch/read methods so my question is why? isn't the same problem everywhere? should not you remove this method from MagicalRecord and modify it appropriately? @hardikdevios, as I know, MagicalRecord is written for the reason of handling background fetches/saves without caring about the internal context management so it's supposed to work on default or private context. Actually, I end up saying that the latest released version of MagicalRecord does not guarantee the management of private/default context when working with background operations.