Shutdown during sync will usually/always fail to write changedIDs file

RESOLVED FIXED in Firefox 53

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tcsc, Assigned: lina)

Tracking

unspecified
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
This is because we wait 1s before writing the file, which by that time will be too late.

The easiest solution for this is probably to use the new JsonFile module here.
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8819131 [details]
Bug 1319175 - Add a `data` setter and `beforeSave` hook to `JSONFile`.

https://reviewboard.mozilla.org/r/98968/#review99258

::: toolkit/modules/JSONFile.jsm:84
(Diff revision 1)
> + *        - beforeSave: Promise-returning function triggered just before the
> + *                      data is written to disk. This can be used to create any
> + *                      intermediate directories before saving.

Perhaps we should define in the comment what happens if beforeSave throws or returns a rejected promise? Do we want it to prevent a write?
Attachment #8819131 - Flags: review?(MattN+bmo) → review+

Comment 4

2 years ago
mozreview-review
Comment on attachment 8819132 [details]
Bug 1319175 - Switch to `JSONFile` for tracker persistence.

https://reviewboard.mozilla.org/r/98970/#review99266

This looks good with the suggested changes, thanks!

::: services/sync/modules/engines.js:55
(Diff revision 1)
>    let level = Svc.Prefs.get("log.logger.engine." + this.name, "Debug");
>    this._log.level = Log.Level[level];
>  
>    this._score = 0;
>    this._ignored = [];
> +  this._store = new JSONFile({

I think `this._store` might end up causing confusion with our "store" object and `this._store` on engines. Maybe `this._storage`?

::: services/sync/modules/engines.js:93
(Diff revision 1)
> +    let basename = OS.Path.dirname(this._store.path);
> +    return OS.File.makeDir(basename, { from: OS.Constants.Path.profileDir });
> +  },
> +
> +  get changedIDs() {
> +    this._store.ensureDataReady();

Isn't ensureDataReady going to be janky for large change sets? It might be better to use its "load" method and promiseSpinningly?

::: services/sync/tests/unit/test_engine.js
(Diff revision 1)
>    engine.wasReset = false;
>    engineObserver.reset();
>    run_next_test();
>  });
>  
> -add_test(function test_invalidChangedIDs() {

ISTM this test might still have value, although changing how the "invalid" data starts on disk would be necessary.
Attachment #8819132 - Flags: review?(markh) → review+
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8819132 [details]
Bug 1319175 - Switch to `JSONFile` for tracker persistence.

https://reviewboard.mozilla.org/r/98970/#review99266

> Isn't ensureDataReady going to be janky for large change sets? It might be better to use its "load" method and promiseSpinningly?

Using `promiseSpinningly` here has an interesting side effect: scheduled async functions (and tasks) will still run in the meantime. In http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/services/sync/tests/unit/test_history_tracker.js#132-133, for example, the `changedIDs` getter will run on the tick *after* `verifyTrackedCount`, meaning the tracker won't see the new value yet.

It's easy to work around; I can avoid calling `Async.promiseSpinningly(this._storage.load())` if the data is already loaded. But I suspect this kind of "synchronously asynchronous" getter might still cause a problem where we bump the score, but `changedIDs` won't have the new change yet.

What do you think is the right approach here? AFAICT, the old persistence didn't really handle this (http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/services/sync/modules/engines.js#55-56,109). Any changes that were added before `loadChangedIDs` would get clobbered when `loadChangedIDs` finished, and any persisted changes would get clobbered if `saveChangedIDs` finished before `loadChangedIDs`.

> ISTM this test might still have value, although changing how the "invalid" data starts on disk would be necessary.

Sure thing. Rewrote the test so that it starts out with invalid data, since I removed the `changedIDs` setter.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8819131 [details]
Bug 1319175 - Add a `data` setter and `beforeSave` hook to `JSONFile`.

https://reviewboard.mozilla.org/r/98968/#review99258

> Perhaps we should define in the comment what happens if beforeSave throws or returns a rejected promise? Do we want it to prevent a write?

I think we do. Sync uses this to create directories; if that fails, the write will fail, anyway. Clarified in the comment and added a test. Thanks!
(Assignee)

Comment 10

2 years ago
Mark, please take another look. This should fix the intermittent failure we were seeing yesterday.
Flags: needinfo?(markh)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8819132 [details]
Bug 1319175 - Switch to `JSONFile` for tracker persistence.

https://reviewboard.mozilla.org/r/98970/#review100368

Looks great, thanks!
(Assignee)

Updated

2 years ago
Flags: needinfo?(markh)

Comment 12

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1febc9e8d6d5
Add a `data` setter and `beforeSave` hook to `JSONFile`. r=MattN
https://hg.mozilla.org/integration/autoland/rev/ced746a2532e
Switch to `JSONFile` for tracker persistence. r=markh

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1febc9e8d6d5
https://hg.mozilla.org/mozilla-central/rev/ced746a2532e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

2 years ago
Depends on: 1325523
You need to log in before you can comment on or make changes to this bug.