Closed Bug 1224531 Opened 5 years ago Closed 4 years ago

Provide a mechanism for the updater to drive kinto collection sync

Categories

(Cloud Services :: Firefox: Common, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

Attachments

(1 file, 1 obsolete file)

Work has been done to integrate kinto servers with balrog so the updater can get information on when collections have been changed.

This is the client part of this work.
s/balrog/kinto "changes" api/
There are two parts to this change. The first is a module to drive kinto
collection sync. This gives server-provided last-update times to each module
managing collection information so that data is only fetched when updates are
necessary. This also keeps track of when pings last took place (for future use)
and any apparent difference between client and server clock (we need this later
for the content signing work).

Currently only one module (the kinto version of the OneCRL client) consumes this
information, though more will follow.

The second is a minor change to nsBlocklistService.js to ensure that this ping
takes place whenever the existing blocklist ping happens.

Review commit: https://reviewboard.mozilla.org/r/37491/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37491/
Attachment #8725440 - Flags: review?(rnewman)
Attachment #8725440 - Flags: review?(dtownsend)
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

https://reviewboard.mozilla.org/r/37491/#review34025

::: services/common/kinto-updater.js:16
(Diff revision 1)
> +const PREF_KINTO_CLOCK_DIFFERENCE = "services.kinto.clock_difference_seconds";

Is seconds fine-grained enough?

I have a gentle preference for suffixing the const names: `_SECONDS`.

Elsewhere in our codebase (particularly in FxA) we use "clock skew" for this, rather than "clock difference"; consider reusing that terminology here.

::: services/common/kinto-updater.js:42
(Diff revision 1)
> +    let serverTime = Date.parse(response.headers.get("Date"));

`serverTimeMillis`. You divide and compare this in several places, so clarity helps!

If the response is missing the `Date` header, `serverTime` will end up as `NaN`. You should null-check `response.headers.get` and bail out in this edge case.

::: services/common/kinto-updater.js:53
(Diff revision 1)
> +        let lastModified = collectionInfo.last_modified;

What happens if `last_modified` is missing?

::: services/common/kinto-updater.js:54
(Diff revision 1)
> +        if (kintoClient.maybeSync) {

Move this check up two lines into the surrounding `if`.

::: services/common/kinto-updater.js:55
(Diff revision 1)
> +          promises.push(kintoClient.maybeSync(lastModified, serverTime));

Future concern: this will cause all of these clients to sync in parallel, particularly -- and most perniciously -- on a first run. Right now you only have one client, so it doesn't matter.

Present concern: using `Promise.all` means we'll reject as soon as *any* client fails to sync.

If one client always runs first and fails first, we'll reject immediately and callers won't wait for subsequent clients. If we do something crazy like run `checkVersions` and block shutdown, this won't block. I doubt that's intended behavior.

I think both of those two things should be changed:

* We should sync these clients sequentially (perhaps with a timeout) to avoid contending for disk or network resources.

* We should wait for all clients to sync before filling or rejecting the returned promise.

I think that means you should be using something like `Promise.mapSeries` here, not `Promise.all`.

::: services/common/tests/unit/xpcshell.ini:13
(Diff revision 1)
> +[test_kinto_updater.js]

Did you forget to `hg add` this file?
Attachment #8725440 - Flags: review?(rnewman)
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/1-2/
Attachment #8725440 - Flags: review?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #3)
> ::: services/common/kinto-updater.js:16
> (Diff revision 1)
> > +const PREF_KINTO_CLOCK_DIFFERENCE = "services.kinto.clock_difference_seconds";
> 
> Is seconds fine-grained enough?

Yes, certainly. We're mostly interested in this for course-grained "are TLS errors because of clock issues" purposes.
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/2-3/
Mossop, I r? you because there's a very tiny change to nsBlocklistService.js - could you take a quick look, please?
Flags: needinfo?(dtownsend)
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

https://reviewboard.mozilla.org/r/37491/#review34865

::: toolkit/mozapps/extensions/nsBlocklistService.js:634
(Diff revision 3)
> +        Components.utils.import("resource://services-common/kinto-updater.js");

Please pass an empty object as the second argument to avoid polluting global scope.
Attachment #8725440 - Flags: review?(dtownsend) → review+
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/3-4/
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/4-5/
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/5-6/
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/6-7/
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/7-8/
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/8-9/
Attachment #8727396 - Attachment is obsolete: true
I had to make some changes to toolkit/mozapps/extensions bits because of test issues. In particular, the checkVersions call results in connections being made to the collections server which, because the default is a non-local host, causes test failures.

I spent some time this morning discussing options with :Unfocused - initially I looked at disabling kinto update in the tests were Blocklist.notify is invoked - but it seemed better to instead have the kinto-updater ping a dummy local URL.
https://reviewboard.mozilla.org/r/38469/#review35047

::: testing/profiles/prefs_general.js:110
(Diff revision 1)
> +user_pref("services.kinto.base", "http://localhost/dummy-kinto/v1");

Any reason this doesn't follow the same %(server)s/ pattern that every other URL in this file does?

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_regexp.js:65
(Diff revision 1)
> +  Services.prefs.setCharPref("services.kinto.base",

These should probably all reference gPort.
(In reply to Richard Newman [:rnewman] from comment #21)
> https://reviewboard.mozilla.org/r/38469/#review35047
> 
> ::: testing/profiles/prefs_general.js:110
> (Diff revision 1)
> > +user_pref("services.kinto.base", "http://localhost/dummy-kinto/v1");
> 
> Any reason this doesn't follow the same %(server)s/ pattern that every other
> URL in this file does?

No reason, I'll fix that...

> ::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_regexp.js:65
> (Diff revision 1)
> > +  Services.prefs.setCharPref("services.kinto.base",
> 
> These should probably all reference gPort.

Ultimately, yes - though there's no actual server (yet - at most we'd have an empty changes collection in any case). The intent here is to prevent update pings in unrelated tests causing test failures. I can add gPort, though update pings will just result in 404s (and, thus, rejection of the promise returned by checkVersions).
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37491/diff/9-10/
Attachment #8725440 - Flags: review?(rnewman) → review+
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

https://reviewboard.mozilla.org/r/37491/#review35467

If this works, fine by me.
Attachment #8725440 - Flags: review?(dkeeler)
Flags: needinfo?(dtownsend)
Comment on attachment 8725440 [details]
MozReview Request: Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r?rnewman,mossop

https://reviewboard.mozilla.org/r/37491/#review35635

I reviewed the security/ changes. Was there anything else you wanted me to look at?
(In reply to David Keeler [:keeler] (use needinfo?) from comment #26)
> I reviewed the security/ changes. Was there anything else you wanted me to
> look at?


No, thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed64dabb118de86c28e2f6a05c61d42f3e7e096
Bug 1224531 - Provide a mechanism for the updater to drive kinto collection sync r=rnewman,mossop
https://hg.mozilla.org/mozilla-central/rev/8ed64dabb118
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1259145
Depends on: 1261271
You need to log in before you can comment on or make changes to this bug.