Need to find the card!
tracking-fxios: ? → 2.0+
We should have up to 10 mins of background execution time when the user leaves the app to run a long-running background task to perform the sync: https://developer.apple.com/library/ios/documentation/iPhone/Conceptual/iPhoneOSProgrammingGuide/BackgroundExecution/BackgroundExecution.html Shouldn't take longer than 10 mins. Need to double check to see what we're allowed to do during that time.
Summary: Sync on exit and/or soon after launch → Sync on exit
Is this as simple as calling syncEverything() in the AppDelegate, then putting the existing shutdown in an upon block? https://github.com/mozilla/firefox-ios/blob/master/Client/Application/AppDelegate.swift#L293
Assignee: nobody → jhugman
Status: NEW → ASSIGNED
Almost. Two big things to think about: the green light and DB closure. Syncs can take an unbounded amount of time. For that reason the caller provides a 'green light' function. The one we use now limits any sync to one minute and foreground-only: if we've exceeded the time limit or entered the background when the sync operation checks in, it'll gracefully give up on the rest of the sync. https://github.com/mozilla/firefox-ios/blob/873459e170ba8d5fac47ee8fad1f20bf726ab3d3/Providers/Profile.swift#L966-L966 Running a background sync will involve changing that behavior, otherwise we'll immediately abort the new sync! We can probably remove the foreground-only constraint altogether, but to do so we will ought to make sure we don't forcibly shut down the profile (closing its databases) until a sync — either one that's already in progress when we background, or one we trigger in the background — is done. The existing shutdown code attempts to close each of our two databases by nilling out the shared DB connection. (The deinitializer for the connection instance will take care of the rest.) Our hypothesis is that this is relatively safe in the presence of an existing sync, but we almost certainly do leave a connection open sometimes, and we don't want to close these then immediately reopen them. It's not enough to fire off a sync with an upon -- we might already be syncing, and so we need to grab the existing sync's Deferred if one exists. To do that, calls to syncEverything (or individual syncs, like the one we trigger in onLoginDidChange) should put their `Deferred` results somewhere, so there's always a hook to chain off an ongoing sync. Right now we use a spinlock (syncLock in Profile.swift) to track whether a sync is in progress. That should probably be some kind of atomic property instead.
Created attachment 8718560 [details] [review] Pull request This feels like it's on the right track, but I haven't done anything with atomicity/spinlocks yet. I'm also not happy with having needing an extra method (or its name) on the SyncManager.
Attachment #8718560 - Flags: feedback?(rnewman)
Comment on attachment 8718560 [details] [review] Pull request Comments on the PR. I think everything should be pushed down into `syncSeveral`; callers shouldn't be aware that a sync is in process, so on exit we can just call self.profile.syncManager.syncEverything() and if a sync for all engines is in progress, we'll just return that; if not, we'll chain off the existing sync to sync the remainder; if there isn't one, we'll run a new full sync.
Attachment #8718560 - Flags: feedback?(rnewman) → feedback+
Please stop spamming bugs.
Test scenarios: * Background the app with no account set up. Nothing untoward should happen. * Likewise, but with the account in Unverified (create a new account). Sync shouldn't work, because the account isn't syncable. * Start a sync from Settings, then immediately background the app. There should be no additional sync. * Background the app seconds after launch (never synced before). Sync or don't sync, but specify the behavior! * Allow a scheduled sync to run (fifteen seconds after launch) and complete, then background the app within the rate-limiting period. We expect no sync. * Likewise, but outside the rate-limiting period. We expect another sync. * Save a login. Wait for the post-login-change sync to occur, then background the app. We expect another sync to occur immediately, syncing everything but logins.
I've just pushed more commits to PR https://github.com/mozilla/firefox-ios/pull/1526 . It needs some more feedback. rnewman: please could you have a look at it. Also: are there any automated tests for this (it doesn't feel untestable, just that it hasn't been automated yet)?
You could test the sync continuation stuff, but syncing on exit itself is a little more challenging -- I don't think we have good control over the OS lifecycle callbacks.
Attachment #8718560 - Flags: feedback+ → feedback?
Comment on attachment 8718560 [details] [review] Pull request Comments on the PR. Nice work!
Attachment #8718560 - Flags: feedback? → feedback+
I think this is ready for a review now. I've kept using the overall structure approximately the same, while (I think) addressing the feedback from earlier f+s.
Comment on attachment 8718560 [details] [review] Pull request Relatively speaking just nits, but they're the kind of nits that would make some people squint, so happy to take another look at this after. Tests would be super appreciated for this, but adding tests would be difficult without breaking out a little library for intermediate deferreds with accumulation. Consider whether you want to do that so you can test it…
Comment on attachment 8718560 [details] [review] Pull request Left a comment in the PR, but I think this is ready for review.
Comment on attachment 8718560 [details] [review] Pull request Lovely. Really nice to see tests for this! I added my nits and an extra test as four commits here: https://github.com/mozilla/firefox-ios/compare/rnewman/async-reducer?expand=1
Attachment #8718560 - Flags: review?(rnewman) → review+
Addressed nits, tested on device. Merged. Yay!
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
https://github.com/mozilla/firefox-ios/commit/9df6ce66e1dd43d86a21ca6d71836984b0aece42 Thanks for your patience, James!
status-fxios-v3.0: --- → wontfix
status-fxios-v4.0: --- → fixed
tracking-fxios: 4.0+ → +
Target Milestone: --- → 4.0
You need to log in before you can comment on or make changes to this bug.