1732792 - retry polling requests without proxy

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, enhancement, P1)

Description

[Shane Caraveo (:mixedpuppy)]

a bypassProxy flag has been added in https://phabricator.services.mozilla.com/D126550

Remote settings should re-poll if there is a failure, and the request was using a proxy.

Straw man changes

 remoteSettings.pollChanges = async ({
    expectedTimestamp,
    trigger = "manual",
    full = false,
    bypassProxy = false,
  } = {}) => {

and later in that

    try {
      pollResult = await Utils.fetchLatestChanges(Utils.SERVER_URL, {
        expectedTimestamp,
        lastEtag,
        bypassProxy, // fetchLatestChanges would pass this through to ServiceRequest
      });
    } catch (e) {
      if (!bypassProxy && pollResult.request.channel.proxyInfo) {
        remoteSettings.pollChanges(expectedTimestamp, trigger, full, true);
        return;
      }
      // Report polling error to Uptake Telemetry.
      let reportStatus;
      if (/JSON\.parse/.test(e.message)) {
        reportStatus = UptakeTelemetry.STATUS.PARSE_ERROR;
      } else if (/content-type/.test(e.message)) {
        reportStatus = UptakeTelemetry.STATUS.CONTENT_ERROR;
      } else if (/Server/.test(e.message)) {
        reportStatus = UptakeTelemetry.STATUS.SERVER_ERROR;
      } else if (/Timeout/.test(e.message)) {
        reportStatus = UptakeTelemetry.STATUS.TIMEOUT_ERROR;
      } else if (/NetworkError/.test(e.message)) {
        reportStatus = UptakeTelemetry.STATUS.NETWORK_ERROR;
      } else {
        reportStatus = UptakeTelemetry.STATUS.UNKNOWN_ERROR;
      }
      await UptakeTelemetry.report(
        TELEMETRY_COMPONENT,
        reportStatus,
        pollTelemetryArgs
      );
      // No need to go further.
      throw new Error(`Polling for changes failed: ${e.message}.`);
    }

Comment 1

[Stuart Colville [:muffinresearch]]

Hi Mathieu, we'd like to target Fx95 for this change, are you ok with that timeline?

Comment 2

[Mathieu Leplatre [:leplatrem]]

Yes, I assume I will be able to do it. I will start working this week.

Comment 3

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1732792 - Retry polling requests without proxy r?mixedpuppy

Comment 4

[Rob Wu [:robwu]]

I'm taking over from Mathieu.

Currently writing unit tests, I may revisit the implementation if it makes sense to do so.

I'll also check a pref, depending on what we do for bug 1734435.

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1732792 - Fall back to direct request upon fetch failure in RemoteSettings

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1732792 - Record telemetry for fallback in Utils.fetch

Telemetry definition and data review is in bug 1733481

Comment 7

[Rob Wu [:robwu]]

The patches here depend on bug 1733481 (for the changes in ServiceRequest.jsm).

To make sure that this patch stack of two only applies if the other patches stick (opposed to being backed out), I've made a change to code that was changed in that other patch: https://phabricator.services.mozilla.com/D129645?vs=on&id=501479#C4438550OL77

The change itself is useful on its own (using internal.bypassProxy = true instead of internal.bypassProxy = this.bypassProxyEnabled, because the if-block that contains it already checks the this.bypassProxyEnabled condition).

Locally, I've verified that the new tests pass on macOS and on Android. The try push is green: https://treeherder.mozilla.org/jobs?repo=try&revision=7276c6021ceaa0e87d04f201788d28668dbaa2c0
So once the second patch has been approved, I'll push it to Lando.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/600e6dfd7436
Fall back to direct request upon fetch failure in RemoteSettings r=leplatrem,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/309190b4a49d
Record telemetry for fallback in Utils.fetch r=mixedpuppy

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/197af0b52aec
Backed out 3 changesets (bug 1732792, bug 1733481) for causing talos crashes in ConditionVariableImpl and xpcshell failures in test_remote_settings_utils.js.

Comment 10

[Alexandru Michis [:malexandru]]

Backed out 3 changesets (Bug 1732792, Bug 1733481) for causing talos crashes in ConditionVariableImpl and xpcshell failures in test_remote_settings_utils.js.
Backout link: https://hg.mozilla.org/integration/autoland/rev/197af0b52aec261be5f484b48c46c26c8f208ae2
Push with failures, failure log.

Comment 11

[Rob Wu [:robwu]]

The test failures in test_remote_settings_utils.js etc. are because the second patch in this stack calls request.logProxySource. However, the method is not an instance method but a static method. This is fixed in https://phabricator.services.mozilla.com/D129683?vs=501480&id=501585#toc

I'll check why the Talos tests crash, and if that doesn't have anything to do with the patch stack here, I'll reland everything.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1a755a61abf
Fall back to direct request upon fetch failure in RemoteSettings r=leplatrem,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6d9a180e160f
Record telemetry for fallback in Utils.fetch r=mixedpuppy

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d1a755a61abf
https://hg.mozilla.org/mozilla-central/rev/6d9a180e160f