Closed Bug 1331629 Opened 7 years ago Closed 7 years ago

Handle Backoff headers in updater

Categories

(Toolkit :: Blocklist Implementation, defect)

Product:
Toolkit
Component:
Blocklist Implementation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

Details

Attachments

(2 files)

Bug 1331629 - Reuse the same kinto client instance accross syncs ()
59 bytes, text/x-review-board-request
mgoodwin
: review+
Details
Bug 1331629 - Handle Backoff header in blocklist updater ()
59 bytes, text/x-review-board-request
mossop
: review+
Details 
      No description provided.
Assignee: nobody → mathieu
Comment on attachment 8828328 [details]
Bug 1331629 - Handle Backoff header in blocklist updater ()

I'll take this review
Attachment #8828328 - Flags: review?(MattN+bmo) → review?(dtownsend)
Comment on attachment 8828328 [details]
Bug 1331629 - Handle Backoff header in blocklist updater ()

https://reviewboard.mozilla.org/r/105790/#review107698

::: services/common/blocklist-updater.js:33
(Diff revision 1)
>  
>  // Add a blocklist client for testing purposes. Do not use for any other purpose
>  this.addTestBlocklistClient = (name, client) => { gBlocklistClients[name] = client; }
>  
> +
> +let _backoffReleaseTime = null;

I don't know the spec for this feature. Are we happy with the backoff getting reset if the user restarts their browser? If not this should go in a pref instead of a global.

::: services/common/blocklist-updater.js:42
(Diff revision 1)
>  this.checkVersions = function() {
>    return Task.spawn(function* syncClients() {
> +
> +    // Check if the server backoff time is elapsed.
> +    if (_backoffReleaseTime) {
> +      const remainingMilliseconds = _backoffReleaseTime - new Date().getTime();

Use Date.now() instead of new Data().getTime().

::: services/common/blocklist-updater.js:73
(Diff revision 1)
>  
>      const response = yield fetch(changesEndpoint, {headers});
>  
> +    // Check if the server asked the clients to back off.
> +    if (response.headers.has("Backoff")) {
> +      const backoffSeconds = parseInt(response.headers.get("Backoff"), 10);

Check the return value here with isNaN and ignore it if that happens. It looks like that is the behaviour anyway but I'd rather we were explicit about checking the server response than relying on how the current checks are done.

::: services/common/blocklist-updater.js:74
(Diff revision 1)
>      const response = yield fetch(changesEndpoint, {headers});
>  
> +    // Check if the server asked the clients to back off.
> +    if (response.headers.has("Backoff")) {
> +      const backoffSeconds = parseInt(response.headers.get("Backoff"), 10);
> +      _backoffReleaseTime = new Date().getTime() + backoffSeconds * 1000;

Date.now() again.

::: services/common/tests/unit/test_blocklist_updater.js:150
(Diff revision 1)
> +  try {
> +    yield updater.checkVersions();
> +  } catch (e) {
> +    error = e;
> +  }
> +  do_check_eq(error.message, "Server is asking clients to back off; retry in 10s.");

It only takes a tiny amount of time to pass before that 10s becomes 9s so I think there is an intermittent failure waiting to happen here. Just check that the first part of the message matches.

::: services/common/tests/unit/test_blocklist_updater.js:150
(Diff revision 1)
> +  try {
> +    yield updater.checkVersions();
> +  } catch (e) {
> +    error = e;
> +  }
> +  do_check_eq(error.message, "Server is asking clients to back off; retry in 10s.");

You'll get a confusing failure message if the backoff is ignored (something about error.message being undefined I expect). Instead put the message check in the catch block and add a do_check_false(true) at the end of the try block to make it clear that a place we never expected to reach was reached.
Attachment #8828328 - Flags: review?(dtownsend) → review-
Thanks :mossop for you review! I addressed the comments you suggested, it should be fine now.
Attachment #8828327 - Flags: review?(mgoodwin)
Attachment #8828327 - Flags: review?(eglassercamp)
Comment on attachment 8828328 [details]
Bug 1331629 - Handle Backoff header in blocklist updater ()

https://reviewboard.mozilla.org/r/105790/#review109000

This is basically there but I'd like to see one extra part to the test before I sign off on this. The other changes aren't as important but I think make things a bit cleaner. Thanks.

::: services/common/blocklist-updater.js:45
(Diff revision 2)
> +    if (Services.prefs.prefHasUserValue(PREF_SETTINGS_SERVER_BACKOFF)) {
> +      const backoffReleaseTime = Services.prefs.getCharPref(PREF_SETTINGS_SERVER_BACKOFF);
> +      const remainingMilliseconds = parseInt(backoffReleaseTime, 10) - Date.now();
> +      if (remainingMilliseconds > 0) {
> +        throw new Error(`Server is asking clients to back off; retry in ${Math.ceil(remainingMilliseconds / 1000)}s.`);
> +      }

I think we should clear the pref when it has expired here. That way if the update check fails for any reason we won't bother to check the backoff time a second time around.

::: services/common/blocklist-updater.js:79
(Diff revision 2)
> +      if (!isNaN(backoffSeconds)) {
> +        const backoffReleaseTime = Date.now() + backoffSeconds * 1000;
> +        Services.prefs.setCharPref(PREF_SETTINGS_SERVER_BACKOFF, backoffReleaseTime);
> +      }
> +    } else {
> +      Services.prefs.clearUserPref(PREF_SETTINGS_SERVER_BACKOFF);

If we're clearing the pref above we don't need to do it here.

::: services/common/tests/unit/test_blocklist_updater.js:151
(Diff revision 2)
> +    // The previous line should have thrown an error.
> +    do_check_true(false);
> +  } catch (e) {
> +    do_check_true(/Server is asking clients to back off; retry in \d+s./.test(e.message));
> +  }
>  });

Now that we have the backoff setting in a pref we can do a follow up test here. Set PREF_SETTINGS_SERVER_BACKOFF to something lower than Date.now() and attempt another check, it should complete and clear the pref. This also leaves the test state clean for if we add more tests to this file in the future.
Attachment #8828328 - Flags: review?(dtownsend)
Comment on attachment 8828327 [details]
Bug 1331629 - Reuse the same kinto client instance accross syncs ()

https://reviewboard.mozilla.org/r/105788/#review109034

::: services/common/blocklist-clients.js:72
(Diff revision 2)
>      .sort((a, b) => a.id < b.id ? -1 : a.id > b.id ? 1 : 0);
>  }
>  
>  
> -function fetchCollectionMetadata(collection) {
> -  const client = new KintoHttpClient(collection.api.remote);
> +function fetchCollectionMetadata(collection, options={}) {
> +  const {remote=collection.api.remote} = options;

Is `remote` really optional, or is it something we always expect to be passed? Since we no longer have a `remote` on the Kinto client itself, I feel like we should make this mandatory.

::: services/common/blocklist-clients.js:160
(Diff revision 2)
>      // if there is a signerName and collection signing is enforced, add a
>      // hook for incoming changes that validates the signature
> +    let hooks;
>      if (this.signerName && enforceCollectionSigning) {
> -      opts.hooks = {
> +      hooks = {
>          "incoming-changes": [this.validateCollectionSignature.bind(this)]

It seems like we won't pass `remote` here; is that OK?

::: services/common/blocklist-clients.js:170
(Diff revision 2)
> -      let connection;
> +      let sqliteHandle;
>        try {
> -        connection = yield FirefoxAdapter.openConnection({path: KINTO_STORAGE_PATH});
> -        const db = kintoClient(connection, this.bucketName);
> -        const collection = db.collection(this.collectionName, opts);
> +        // Synchronize remote data into a local Sqlite DB.
> +        sqliteHandle = yield FirefoxAdapter.openConnection({path: KINTO_STORAGE_PATH});
> +        const options = {
> +          remote,

I don't think `remote` does anything here.
Attachment #8828327 - Flags: review?(eglassercamp) → review-
Comment on attachment 8828328 [details]
Bug 1331629 - Handle Backoff header in blocklist updater ()

https://reviewboard.mozilla.org/r/105790/#review110688
Attachment #8828328 - Flags: review?(dtownsend) → review+
Comment on attachment 8828327 [details]
Bug 1331629 - Reuse the same kinto client instance accross syncs ()

https://reviewboard.mozilla.org/r/105788/#review111038
Attachment #8828327 - Flags: review?(mgoodwin) → review+
Keywords: checkin-needed
Thanks folks for your reviews!
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd195a6fabaf
Reuse the same kinto client instance accross syncs (r=mgoodwin)
https://hg.mozilla.org/integration/autoland/rev/f0f5fdf96c80
Handle Backoff header in blocklist updater (r=mossop)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dd195a6fabaf
https://hg.mozilla.org/mozilla-central/rev/f0f5fdf96c80
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: