Closed Bug 1412351 Opened 2 years ago Closed 2 years ago

Sync should check for shutdown more frequently.

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(1 file)

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).
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.
Assignee: nobody → tchiovoloni
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 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!
Attachment #8922906 - Flags: review?(kit) → review+
Priority: -- → P1
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)
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/275d752d44cc
Make sync behave better during shutdown r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/275d752d44cc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
See Also: → 1416788
Summary: Sync hould check for shutdown more frequently. → Sync should check for shutdown more frequently.
You need to log in before you can comment on or make changes to this bug.