drewmccormack / ensembles

A synchronization framework for Core Data.
MIT License
1.63k stars 131 forks source link

Adds download tracking using NSFileCoordinator #102

Closed fdstevex closed 10 years ago

fdstevex commented 10 years ago

This sequentially waits for all the started downloads to complete. There's no guarantee that when all the downloads are done all the files really are still "downloaded" (maybe they get deleted, etc) but since this is just a hint to the app that now is a good time to sync, seeing that all the started downloads have completed should suffice.

The problem this fixes is that neither of the delegate methods are firing when downloads complete.

drewmccormack commented 10 years ago

Think there may be a few problems with the request. The metadata notif often fires multiple times, so count probably won't be right.

Also notifying on background thread, I think.

I think I know why original code wasn't working. Simply a logic error. Will fix and try that first. Would prefer to stick with metadata if possible, since we have to use that to initiate downloads.

If that really cannot be made to work, I'll change to the file coord. Thanks.

drewmccormack commented 10 years ago

Fixed by adapting metadata query code. Thanks for the request. It might still be useful in future.

fdstevex commented 10 years ago

I didn't test your changes yet but from what I was observing, it won't make a difference.

I set a breakpoint on initiateDownloads, where it calls startDownloadingUbiquitousItemAtURL: as well as breakpoints on metadataQueryDidUpdate and metadataQueryDidFinishGathering. After the last startDownloadingUbiquitousItemAtURL call, there simply are no calls to the other two, so no opportunity to fire the notification using that method.

The notification firing multiple times is okay - that's why there's the count. If three notifications come in, the count goes to three. The operation queue starts waiting on the first one, then the second, then the third, updating the count as they complete (even if they don't complete in the same order, that's okay). If more notifications come in while one of them is downloading, then the count goes up and the notification isn't sent until all the downloads complete.

Notifications and threads - I assumed it's the job of the notification recipient to ensure they're on the right thread. Other notifications are posted from the eventintegrator queue.

The down side to the NSFileCoordinator method is it's synchronous. That's why I created the queue and set the maxConcurrentOperationCount to 1, so there's only one thread sitting waiting for a coordinator to complete.

drewmccormack commented 10 years ago

Yes, I realized your count should be fine when I looked closer.

I tested the code this morning before posting, and it was firing the notification each time for me. The original code had a flaw: if the gathering phase ended with a non-zero result, it was not restarting the query, which you have to do in order for it to drop to zero.

My tests indicate it is working now, but let me know if it isn't and I will consider the file coordinator. I would rather stick with one mechanism if possible, and we can't avoid the metadata query.

Re: notifications, I nearly always post to the main thread. Very few developers would realize they are on the wrong thread and could lead to some nasty bugs, especially since the merge... methods etc have to be called on the main thread.

drewmccormack commented 10 years ago

I just saw your comment about event integrator notifs: I post on main thread whenever possible, and only on background threads where it is necessary. The integrator does use background threads for one or two delegate methods/notifs, but that is because they include a context that the user may wish to interact with.

I tossed up whether to fire some of these methods on the main thread, but didn't, because if you are using locking of your context or something like that, it is very likely to deadlock. So I deliberately use the background queue. Not completely settled though, so if you think the main thread would be better, I'm happy to consider that.

fdstevex commented 10 years ago

I tested your change and it is working. Excellent - I much prefer that to the method I was using. Thanks.

fdstevex commented 10 years ago

I spoke too soon. Saw it happen again just now, where downloads were started, but no notification was sent.

Put a breakpoint on the startDownloading line to log a message, and a different breakpoint to do the same on the line that sends a notification, and watch for started downloads that don't result in a notification. It still happens, although not always.

drewmccormack commented 10 years ago

OK, I'll put in the FC then.