Closed Bug 1319175 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: tcsc, Assigned: lina)

References

Details

Attachments

(2 files)

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.
Priority: -- → P2
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Priority: P2 → P1
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 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+
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 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!
Mark, please take another look. This should fix the intermittent failure we were seeing yesterday.
Flags: needinfo?(markh)
Comment on attachment 8819132 [details]
Bug 1319175 - Switch to `JSONFile` for tracker persistence.

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

Looks great, thanks!
Flags: needinfo?(markh)
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
https://hg.mozilla.org/mozilla-central/rev/1febc9e8d6d5
https://hg.mozilla.org/mozilla-central/rev/ced746a2532e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1325523
You need to log in before you can comment on or make changes to this bug.