1168504 - Sync on exit

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Comment 1

[Richard Newman [:rnewman]]

Need to find the card!

Comment 2

[Stephan Leroux [:sleroux]]

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.

Comment 3

[James Hugman [:jhugman] [@jhugman]]

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

Comment 4

[Richard Newman [:rnewman]]

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.

Comment 5

[James Hugman [:jhugman] [@jhugman]]

Attachment: 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.

Comment 6

[Richard Newman [: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.

Comment 7

[juancasas]

Bien

Comment 8

[juancasas]

Regula

Comment 9

[Aaron Train [:aaronmt]]

Please stop spamming bugs.

Comment 10

[Richard Newman [:rnewman]]

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.

Comment 11

[James Hugman [:jhugman] [@jhugman]]

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)?

Comment 12

[Richard Newman [:rnewman]]

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.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8718560 [details] [review]
Pull request

Comments on the PR. Nice work!

Comment 14

[James Hugman [:jhugman] [@jhugman]]

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 15

[Richard Newman [:rnewman]]

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 16

[James Hugman [:jhugman] [@jhugman]]

Comment on attachment 8718560 [details] [review]
Pull request

Left a comment in the PR, but I think this is ready for review.

Comment 17

[Richard Newman [:rnewman]]

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

Comment 18

[James Hugman [:jhugman] [@jhugman]]

Addressed nits, tested on device.

Merged. Yay!

Comment 19

[Richard Newman [:rnewman]]

https://github.com/mozilla/firefox-ios/commit/9df6ce66e1dd43d86a21ca6d71836984b0aece42

Thanks for your patience, James!