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

Make saveWithBlock: use saveNestedContexts #317

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

As you maybe know I was having quite some trouble with MagicalRecord and having it persist my data to disk. (See issue #314.) As it turns out I made some mistakes myself, but there is also an inconsistency between saveWithBlock: and saveInBackgroundWithBlock:.

The saveInBackgroundWithBlock: is using the saveNestedContexts methods and is persisting stuff to disk as expected, but when I switched to saveWithBlock: I lost data because it was using the not nested save methods.

In my opinion the behavior should be consistent to prevent confusing people (like me) and losing their data.

I hope one of the devs has some time to pull this in and save me and other a lot of time hunting down persistence issues. If there is anything I should change or improve before pulling these commits in let me know!

mac-cain13 added some commits Nov 25, 2012

Make saveWithBlock: save the nested contexts instead of just it's loc…
…alContext

This makes behavior consistent with the saveInBackground-methods and fixes issue #314
Made saveWithBlock:completion:errorHandler: more like saveInBackgroun…
…dUsingContext:block:completion:errorHandler:

Mainly to prevent confusion and improve maintainability. This will make it more obvious what is going on and what the differences between the two are.

Okay, I was a bit to quick submitting this pull request. Since saveNestedContexts is non-blocking the save will not complete before retuning. Because of this you'll end up with objects with temporary IDs that will not be resolved correctly. Using a blocking version of saveNestedContext causes a deadlock, so that is a no go.

Do you guys think this saveWithBlock: vs saveInBackgroundWithBlock: behavior thing is a problem that should be solved? And if yes, what approach should we take?
For now the only option/workaround I see is to always use background saving and avoid using the saveWithBlock: method...

Contributor

tonyarnold commented Nov 25, 2012

I was certain that this issue was fixed by 870ca22

Contributor

tonyarnold commented Nov 25, 2012

But… it looks like it was merged into master, not develop. thunk

When I'm back at my Mac, I'll see if the fix can still be merged easily via a cherry pick or some such.

Member

blackgold9 commented Nov 25, 2012

Doh! Totally my fault! My apologies!


From: Tony Arnoldmailto:notifications@github.com
Sent: ‎11/‎25/‎2012 12:47 PM
To: magicalpanda/MagicalRecordmailto:MagicalRecord@noreply.github.com
Subject: Re: [MagicalRecord] Make saveWithBlock: use saveNestedContexts (#317)

But… it looks like it was merged into master, not develop. thunk

When I'm back at my Mac, I'll see if the fix can still be merged easily via a cherry pick or some such.


Reply to this email directly or view it on GitHub:
#317 (comment)

Alright, that makes sense. I'm slowly getting there, it's quite confusing with the persisting stuff going wrong and not knowing all the ins and outs of MagicalRecord, but I'm learning fast while digging into this issues. ;) So this commit + the permanent ID fetching fix from the development branch should make it work for me.

Just to be sure, with this commit @tonyarnold refers to, MR_save is just a blocking variant of MR_saveNestedContexts isn't it? Looking at the code they both traverse up through all parent contexts saving each of them, finally saving the root context to disk. Or am I interpreting this incorrectly?

Contributor

tonyarnold commented Nov 25, 2012

@mac-cain13 yes, it should block (and there's a test in d41d744 that should verify that).

Okay, since this is already fixed in master this pull request is superfluous. Also the code in this pull request doesn't work correctly, so I'll close this.

@blackgold9 could you make sure commit 870ca22 will also be in develop to prevent further confusion?

@mac-cain13 mac-cain13 closed this Nov 29, 2012

Contributor

tonyarnold commented Nov 29, 2012

@mac-cain13 — I merged it back into develop yesterday. It should be right now.

Nice! Thanks.

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