Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental/autosave freeze fix #25

Open
wants to merge 8 commits into
base: v0.3.x
Choose a base branch
from

Conversation

adib
Copy link
Contributor

@adib adib commented May 27, 2014

This fixes the freezing issue found in #24. Apparently NSDocument freezes during in-place autosave on file coordination, thus the solution is to have our own in-place autosave implementation that skips it. Since the persistent store isn't expected to move while being open anyway, it's likely safe to skip calling NSFileCoordinator during autosave.

I've incorporated this in my application and sent out a beta release and seems stable. Furthermore I'm making that beta live now (cross fingers ;-) )

@mikeabdullah
Copy link
Collaborator

Hi @adib, sorry for the massive delay in getting back to you on this.

I'm really not keen on the solution in this pull request I have to admit. Completing bypassing the standard save method is bound to have bad consequences; if not in your app, at least in mine and others.

What I don't understand is why I'm so far unable to reproduce the hang in our own app. Are you able to reproduce it in a standalone app that I can play with, perhaps?

@adib
Copy link
Contributor Author

adib commented Jun 17, 2014

Frankly reproducing it consistently is a real challenge. The app has a number of background "refresh" jobs and upping the frequency of these refresh jobs significantly increases the probability of the app freezing. The biggie is "probability", means it doesn't always freeze. But it does happen intermittently.

Probably your app (Sandvox?) doesn't have asynchronous background refreshes that writes to Core Data, which makes it difficult for you to reproduce.

So at 30 seconds refresh interval and two different data files (which amounts to four refreshes per 30 seconds since each data file handles two "accounts"), the app usually freezes before 10 minutes of run time on my 2011 MacBook Air (which is dual core with hyper-threading). Whenever it freezes, it's always deadlocks on the stack trace that I've pasted in #24. But the freezing stops after I added this patch. Some of my users also confirmed that the freezes went away, so I'm pretty confident of the fix.

I could issue a license of the app to you and send you an old copy to play it yourself, if you like. But at this point of time I can't create a standalone app that can predictably reproduce the issue.

In any case Core Data should handle coordination issues by itself, as indicated by WWDC 2014 Extensions Part 2 (session 217).

@mikeabdullah
Copy link
Collaborator

Can you expand a bit more upon what these "background refresh jobs" are actually doing?

My reason for hooking into the file coordinator stuff is not to protect the Core Data file. Instead it's to co-ordinate the handling of content for saving into the document package. Because that all gets stashed into an ivar, it needs to be protected so that there's only ever one use of it happening at once. I tried to hook into NSDocument's API for that to make it all work neatly. Maybe in practice it's actually just coming back to cause more trouble and I should implement a dedicated synchronisation mechanism.

@adib
Copy link
Contributor Author

adib commented Jun 17, 2014

These "background refresh jobs" are polling the server for new data (JSON REST), converting it to Core Data objects and then saving the document.

If what you need is to coordinate the document package (as in intra-process coordination), you may want to leverage a GCD serial queue instead of file coordination. At least for the in-place AutoSave where file coordination is definitely overkill.

@mikeabdullah
Copy link
Collaborator

So you're modifying the MOC, and programatically triggering an autosave? Just trying to make sure you're not saving the MOC directly.

Yeah, I think moving away from file coordination is likely a good idea, and GCD the most sensible choice. I do have an approach built on that that could work. Maybe I haven't thought it through enough yet though:

  1. Call super's implementation of saving
  2. -writeToURL:… is called on background thread
  3. Call unblockUserInterface
  4. That frees up the main thread to continue on to its next bit of work, which will be to grab the content for saving (i.e. save the main context and gather any additional content subclasses want)
  5. Background thread waits on a semaphore for step 4 to finish
  6. Background thread saves the root context and writes to disk

I think the semaphore can be achieved by a quick call to dispatch_sync on the main queue, rather than needing an extra ivar, but not entirely sure about that.

@adib
Copy link
Contributor Author

adib commented Jun 18, 2014

Well it creates a child context of the main context and save the child. Autosave kicks in automatically via the autosave interval. It's not the most performant way to do this, but I didn't want to bother with sibling context coordination when I wrote that.

Do you mean the _contents ivar? My suggestion is not to use concurrent queues for any "background" jobs that accesses ivars but serial queues instead. The entire idea of UIDocument's nested context is primarily just to take the load off the UI thread anyway, and not to maximize core utilization. Thus instead of performAsynchronousFileAccessUsingBlock, you could dispatch_async to an internal serial queue. Otherwise still use performAsynchronousFileAccessUsingBlock but dispatch_sync to the internal queue when accessing ivars .

If you're hesitant to maintain another queue, borrowing the root MOC's is probably a good alternative.

@adib
Copy link
Contributor Author

adib commented Jun 20, 2014

On second thought, a short @synchronized(this) { ... } when accessing the _contents ivar shouldn't cause any noticeable performance bottleneck – we're doing I/O anyway and that should be the bigger time sink.

@mikeabdullah
Copy link
Collaborator

What would this be in your example? It can't be _contents, since @synchronized requires the same object be used each time.

@adib
Copy link
Contributor Author

adib commented Jun 22, 2014

'this' is the BSManagedDocument instance.

@synchronized(this) {
_contents =....
}

Thank you


Sasmito Adibowo
http://cutecoder.org

On 23 Jun, 2014, at 3:00, Mike Abdullah [email protected] wrote:

What would this be in your example? It can't be _contents, since @synchronized requires the same object be used each time.


Reply to this email directly or view it on GitHub.

@Motti-Shneor
Copy link

I had this freeze very frequently in my App - and I did not do any heavy background work. All I've done is to manually trigger autosave every 30 sec. For me - "system imposed" autosaves did work, and manual autosaves - made the app hang. Did not test with it since then - I simply gave up driving the autosave myself. Somehowe the OS does it for me (via another mechanism, of the draft-allways-saved on the side, but you need to hit "Save" to commit the changes to the "real" document).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants