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

Already on GitHub? Sign in to your account

saveInBackgroundUsingContext:block:completion:errorHandler: returns before the context is saved #242

Closed
tibr opened this Issue Sep 4, 2012 · 10 comments

Comments

Projects
None yet
3 participants

tibr commented Sep 4, 2012

When using unique entries, saveInBackgroundUsingContext:block:completion:errorHandler: has a problem:
The core data operations are dispatched on the serial action_queue, which is great so I should be sure that different objects with the same unique identifier cannot be created from multiple threads.
However, after the block is executed it calls [localContext MR_saveInBackgroundErrorHandler:errorHandler completion:completion]; which calls performBlock: internally. As far as I know, performBlock: is asynchronous and returns immediately.
The result is that the block completes before the context is saved, making it possible for another block to be perfomed on the action_queue creating an object with the same unique identifier because the previous object has not been saved before the block started execution.

My suggestion would be to swap
`[localContext MR_saveInBackgroundErrorHandler:errorHandler completion:completion];``
with

[localContext MR_saveErrorHandler:errorHandler];
if (completion) {
    completion();
}

What do you think? Good idea? If so, I'll make a pull request...

Member

blackgold9 commented Sep 4, 2012

There are actually several pull requests fixing this in the queue already. If I were you, I'd implement the fix locally while you wait for saul to accept one of them.
On Sep 4, 2012, at 12:55 AM, Tim Brückmann notifications@github.com wrote:

When using unique entries, saveInBackgroundUsingContext:block:completion:errorHandler: has a problem:
The core data operations are dispatched on the serial action_queue, which is great so I should be sure that different objects with the same unique identifier cannot be created from multiple threads.
However, after the block is executed it calls [localContext MR_saveInBackgroundErrorHandler:errorHandler completion:completion]; which calls performBlock: internally. As far as I know, performBlock: is asynchronous and returns immediately.
The result is that the block completes before the context is saved, making it possible for another block to be perfomed on the action_queue creating an object with the same unique identifier because the previous object has not been saved before the block started execution.

My suggestion would be to swap
[localContext MR_saveInBackgroundErrorHandler:errorHandler completion:completion];`
with

[localContext MR_saveErrorHandler:errorHandler];
if (completion) {
completion();
}
What do you think? Good idea? If so, I'll make a pull request...


Reply to this email directly or view it on GitHub.

Member

blackgold9 commented Sep 4, 2012

That is to say, yes, you're hitting a known problem, and you understood it correctly :)

On Sep 4, 2012, at 12:55 AM, Tim Brückmann notifications@github.com wrote:

When using unique entries, saveInBackgroundUsingContext:block:completion:errorHandler: has a problem:
The core data operations are dispatched on the serial action_queue, which is great so I should be sure that different objects with the same unique identifier cannot be created from multiple threads.
However, after the block is executed it calls [localContext MR_saveInBackgroundErrorHandler:errorHandler completion:completion]; which calls performBlock: internally. As far as I know, performBlock: is asynchronous and returns immediately.
The result is that the block completes before the context is saved, making it possible for another block to be perfomed on the action_queue creating an object with the same unique identifier because the previous object has not been saved before the block started execution.

My suggestion would be to swap
[localContext MR_saveInBackgroundErrorHandler:errorHandler completion:completion];`
with

[localContext MR_saveErrorHandler:errorHandler];
if (completion) {
completion();
}
What do you think? Good idea? If so, I'll make a pull request...


Reply to this email directly or view it on GitHub.

Member

blackgold9 commented Sep 6, 2012

Fixed now.

@blackgold9 blackgold9 closed this Sep 6, 2012

tibr commented Sep 10, 2012

Can you show me where this has been fixed? I don't see it in the recent commits and am still having the same issue when trying.

Member

blackgold9 commented Sep 10, 2012

  • (void) MR_saveInBackgroundErrorHandler:(void (^)(NSError *))errorCallback completion:(void (^)(void))completion;
    {
    [self performBlock:^{
    // Save the context
    [self MR_saveWithErrorCallback:errorCallback];

    // If we're the default context, save to disk too (the user expects it to persist)
    if (self == [[self class] MR_defaultContext])
    {
        [[[self class] MR_rootSavingContext] MR_saveInBackgroundErrorHandler:errorCallback completion:completion];
    } else {
        // If we are not the default context (And therefore need to save the root context, do the completion action if one was specified
        if (completion)
        {
            dispatch_async(dispatch_get_main_queue(), completion);
        }
    }
    

    }];
    }

That code will save the default context, then call itself again to save the root context, THEN call the completion handler.
On Sep 10, 2012, at 3:17 AM, Tim Brückmann notifications@github.com wrote:

Can you show me where this has been fixed? I don't see it in the recent commits and am still having the same issue when trying.


Reply to this email directly or view it on GitHub.

Member

blackgold9 commented Sep 10, 2012

That is this commit :
22311b7
On Sep 10, 2012, at 3:17 AM, Tim Brückmann notifications@github.com wrote:

Can you show me where this has been fixed? I don't see it in the recent commits and am still having the same issue when trying.


Reply to this email directly or view it on GitHub.

tibr commented Sep 17, 2012

I might be wrong here but performBlock: inside MR_saveInBackgroundUsingContext:block:completion:errorHandler breaks the synchronous character of the serial action_queue because it is able to start the next operation before the save was finished.

An example:

I do a lot of stuff inside a block, the block finishes and calls MR_saveInBackgroundErrorHandler:completion: which calls performBlock: and immediately returns. The block dispatched to the action_queue finishes while the localContext continues with the save operation. Of course, the completion block is not called until the save operation is finished. However, another block can be started on the action_queue with a new localContext which does not include the saved changes from the previous block.

To verify that, I changed line 104 of NSManagedObjectContext+MagicalSaves.m from
[self performBlock:^{
to
[self performBlockAndWait:^{
which gives the correct behavior. Of course this is not the correct solution but it gives the dispatched block the synchronous character it should have in my opinion. I think the action_queue should not be able to start another operation until the previous changes have been saved correctly.

Owner

casademora commented Sep 17, 2012

I can see your point here. As I am looking at the commit history, it was actually the way you describe it previously....

We'll have to look into this one for a bit...

On Sep 17, 2012, at 2:54 AM, Tim Brückmann notifications@github.com wrote:

I might be wrong here but performBlock: inside MR_saveInBackgroundUsingContext:block:completion:errorHandler breaks the synchronous character of the serial action_queue because it is able to start the next operation before the save was finished.

An example:

I do a lot of stuff inside a block, the block finishes and calls MR_saveInBackgroundErrorHandler:completion: which calls performBlock: and immediately returns. The block dispatched to the action_queue finishes while the localContext continues with the save operation. Of course, the completion block is not called until the save operation is finished. However, another block can be started on the action_queue with a new localContext which does not include the saved changes from the previous block.

To verify that, I changed line 104 of NSManagedObjectContext+MagicalSaves.m from
[self performBlock:^{
to
[self performBlockAndWait:^{
which gives the correct behavior. Of course this is not the correct solution but it gives the dispatched block the synchronous character it should have in my opinion. I think the action_queue should not be able to start another operation until the previous changes have been saved correctly.


Reply to this email directly or view it on GitHub.

tibr commented Sep 17, 2012

Thanks Saul, I ended up using the solution proposed in my first post on the issue:
Changing
[localContext MR_saveInBackgroundErrorHandler:errorHandler completion:completion];
to

[localContext MR_saveErrorHandler:errorHandler];
if (completion) {
    completion();
}

Works great so far...

tibr commented Oct 1, 2012

I think this issue should be opened again.

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