Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

MR_save breaking change in 2.0.8 #318

Closed
batkuip opened this Issue · 14 comments

8 participants

@batkuip

Hi,

I just upgrade from 2.0.7 to 2.0.8 and nothing got persisted to disk anymore. Turns out MR_save doesn't save its parent context anymore. I tried changing all the MR_save's to MR_saveNestedContexts but that is causing major concurrency issues. The old MR_save was using performBlockAndWait and MR_saveNestedContext is using performBlock.

How do I fix this without significant modifications?

Seems silly but MR_save now doesn't work as expected out of the box anymore. The most basic app, set up a sqlite stack, create an entity and then save using MR_save does not work as expected. You have to call MR_saveNestedContext to persist instead. What would you use MR_save for then?

Also, are you following semantic versioning with MR? Seems to me this kind of change should be in 2.1.x

@blackgold9
Collaborator
@batkuip

Any idea on how to easily fix this? I have reverted to 2.0.7 currently with no real options. Actually what is the pattern if I want to do a nested sync save?

To me it make more sense that MR_save would persist to disk out of the box. Same as you were setting up a coredata stack yourself and using the normal "save" method. Anything else just causes major confusion.

@ekurutepe

@batkuip using MR_saveNestedContexts instead of MR_save saves the parent context up to the root.

@blackgold9
Collaborator

@batkuip the intended pattern is saveNEstedContexts. It has a completion handler if you need to do things after the save is successful. It is non-blocking currently to help guide people into not creating blocking ui by default.

@batkuip

I understand where you are coming from, but using a completion handler is not always an option. See example in Issue 320. How would you rewrite that? Having a simple sync save method is essential. Now MR forces you to write horrible convoluted code simply to ensure changes are persisted to disk? That can't be right.

Further more saveNestedContexts is not working reliably from a background thread (issue 320). Which leaves no option to reliably save to disk in 2.0.8.

Also it looks like I'm not to only one: Issue 317, Issue 313, Issue 304, Issue 299 are all related.

Please revert back to the original MR_save behaviour until you've sorted out this mess. (also has the added side benefit of not breaking all the blog articles about MR that use MR_save to persist to disk)

@johntmcintosh

To add another use case... I have a situation where I update an app log in -applicationWillResignActive: and then save it (to disk). With a synchronous save, I'm able to make that save before the app is put into the background. However, with the 2.0.8 version of MR_saveNestedContextsErrorHandler:completion:, the app goes into the background before the save is completed (and the completion block is never executed).

For now I'm reverting back to 2.0.7 and MR_save for its synchronous behavior. I support the preference being to do things asynchronously, but would like to keep a synchronous option around for special occasions.

@tonyarnold
Owner

@johntmcintosh is this in an OS X app, or an iOS app?

If it's iOS, you should be wrapping any essential IO in - (UIBackgroundTaskIdentifier)beginBackgroundTaskWithExpirationHandler:. Using a synchronous save guarantees very little if that save takes too long (and your app is subsequently killed by the OS). In my app, I call - (void)endBackgroundTask: inside a save completion block.

I'm not trying to argue endlessly with you guys over this. I do think there's a use case for a synchronous nested context save method. That being said, there are potentially better ways to be doing some of the things you're trying to do.

@johntmcintosh

@tonyarnold good call. I am on iOS, and you're right -- I completely forgot about that. Synchronous discussions aside, I should be wrapping this as a background task in -applicationWillResignActive. Many thanks for the kind reminder!

@bcyng

+1 vote for sync save. its a lot easier to manage a sync save as there are no concurrency issues. and turning it into an async is a lot easier than turning an async into a sync. for fast simple applications e.g. data conversion when u don't want or need to be dealing with async issues and need to be writing to the db often it just makes sense.
any async use cases such as controlling the responsiveness of the UI should be left up to the developer to control.

@dhennessy

This is a very worrying development to me. I've been using MR as some syntactic sugar on top of core data - the API is a bit more concise and it was easy to see how it mapped onto real core data calls. However, this last change causes MR to behave differently to core data and in an unexpected way. In fact, the only way I finally discovered what was going on was by reading the code. What's worse, it was introduced in a minor update.

@tonyarnold
Owner

@dhennessy and others: we're working on an update to make the save methods consistent, and to restore the original behaviour of the base MR_save method. Bear with us — time's at a bit of a premium at the moment.

@fgarcia

@tonyarnold I just mentioned in another issue about not having a 2.0.9 release. I believe that the commit cfccd40 alone could justify that new release. Certainly it solved my problems and probably also for other people who is still complaining for something that is already fixed. I continue developing with that version and have no problems, but I assume that most users only use the master branch or the cocoapods release.

However, I must highlight again, that I am a really happy MagicalRecord user. Thanks!

@tonyarnold tonyarnold was assigned
@tonyarnold
Owner

Right, for anyone who wants to have a look and perhaps provide some feedback before it's merged, there is a large pull request that addresses this and a number of other confusing aspects of saving using MagicalRecord in #341. If/when this pull request is merged, it will form the base of MagicalRecord 2.1.0.

Sorry it has taken a few days to pull this together, and please bear with us as this new code gets the review it needs.

@tonyarnold
Owner

The changes to the save methods have been released as part of MagicalRecord 2.1, which is available now. We've also changed all completion blocks so that they dispatch onto the main thread — updating to this new version and using the new save methods should resolve the issues that you were seeing.

Feel free to reopen this issue if you continue to see the same problems.

@tonyarnold tonyarnold closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.