1412351 - Sync should check for shutdown more frequently.

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

At the moment this will cause a number of hard to diagnose errors to appear everywhere else in sync, and obfuscate the fact that no, this really is just due to the fact that we're shutting down.

This probably is as simple as peppering our code with a few Async.checkAppReady's, (I can't think of a more robust/complete method of doing this -- I'd be open to suggestions though).

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Taking this since I can do it in the background and it shouldn't take long -- admittedly it's probably not a high priority, but it will make our telemetry and logs easier to read in the future.

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1412351 - Make sync behave better during shutdown

Review commit: https://reviewboard.mozilla.org/r/194072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194072/

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

It's not clear to me everywhere we might want this.

This will prevent us from attempting to sync (or login) if we're about to shutdown, and adds several new checks for shutdown, including one before every request (the checks are essentially free if we aren't shutting down).

In the case of history, I noticed that we override applyIncomingBatch without yielding (the way the base applyIncomingBatch does), so I used a jankYielder instead of just calling checkAppReady.

I expect this should catch most cases? (Keep in mind that anywhere we already use a jankYielder, we don't need a check)

Note that a perfect solution here probably doesn't exist without spinning event loops, since that used to allow us to detect it during any expensive operation we were spinning through -- here we will only notice at the next check.

Because of that, it might be worth detecting if we're currently shutting down when we get unrelated errors. If we are, we can probably assume that that is the cause, and be wrong infrequently enough for it to just be noise in the telemetry. I'm not sure where we would put a check like that though, and have everywhere catch it...

We might only need to do this in telemetry and in ErrorHandler though.

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8922906 [details]
Bug 1412351 - Make sync behave better during shutdown

https://reviewboard.mozilla.org/r/194072/#review199158

::: services/sync/modules/engines.js:1765
(Diff revision 1)
>      }
>    },
>  
>    async _sync() {
>      try {
> +      Async.checkAppReady();

I'm side-eying this a little, but I think it makes sense. (Ideally, the different stages should already fail if they try to make requests or do storage work...but we've seen that's not always the case, so this is a good last resort).

::: services/sync/modules/engines/history.js:186
(Diff revision 1)
>      let failed = [];
>  
>      // Convert incoming records to mozIPlaceInfo objects. Some records can be
>      // ignored or handled directly, so we're rewriting the array in-place.
>      let i, k;
> +    let maybeYield = Async.jankYielder();

Let's add a `checkAppReady` to `applyIncomingBatch` in the forms engine, for completeness.

::: services/sync/modules/resource.js:183
(Diff revision 1)
>  
>    _doRequest(action, data) {
>      this._log.trace("In _doRequest.");
>      return new Promise((resolve, reject) => {
> +      // Don't bother starting a network request if we're shutting down.
> +      Async.checkAppReady();

Good catch!

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8922906 [details]
Bug 1412351 - Make sync behave better during shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194072/diff/1-2/

Comment 6

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c06c1c3f7558 -d 84c757c57083: rebasing 432055:c06c1c3f7558 "Bug 1412351 - Make sync behave better during shutdown r=kitcambridge" (tip)
merging services/sync/modules/constants.js
merging services/sync/modules/engines.js
merging services/sync/modules/engines/forms.js
merging services/sync/modules/engines/history.js
merging services/sync/modules/resource.js
merging services/sync/modules/service.js
merging tools/lint/eslint/modules.json
warning: conflicts while merging tools/lint/eslint/modules.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8922906 [details]
Bug 1412351 - Make sync behave better during shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194072/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/275d752d44cc
Make sync behave better during shutdown r=kitcambridge

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/275d752d44cc