Bug 1563180 - Prevent synchronization to be ran in parallel multiple times r=glasserc
☠☠ backed out by 64ef6b0c521c ☠ ☠
authorMathieu Leplatre <mathieu@mozilla.com>
Sat, 06 Jul 2019 16:05:12 +0000
changeset 541210 7aa8180f36eb010d9bc027cee7440773ad4fe0a0
parent 541209 88f4a687f0393b33602c65c36635f58301261366
child 541211 64ef6b0c521c95fc9846e8940633d2aefa7b7df9
push id11533
push userarchaeopteryx@coole-files.de
push dateMon, 08 Jul 2019 18:18:03 +0000
treeherdermozilla-beta@f4452e031aed [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglasserc
bugs1563180
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1563180 - Prevent synchronization to be ran in parallel multiple times r=glasserc Differential Revision: https://phabricator.services.mozilla.com/D36921
services/settings/RemoteSettingsClient.jsm
services/settings/test/unit/test_remote_settings.js
--- a/services/settings/RemoteSettingsClient.jsm
+++ b/services/settings/RemoteSettingsClient.jsm
@@ -188,16 +188,17 @@ class RemoteSettingsClient extends Event
     super(["sync"]); // emitted events
 
     this.collectionName = collectionName;
     this.signerName = signerName;
     this.filterFunc = filterFunc;
     this.localFields = localFields;
     this._lastCheckTimePref = lastCheckTimePref;
     this._verifier = null;
+    this._syncRunning = false;
 
     // This attribute allows signature verification to be disabled, when running tests
     // or when pulling data from a dev server.
     this.verifySignature = true;
 
     // The bucket preference value can be changed (eg. `main` to `main-preview`) in order
     // to preview the changes to be approved in a real client.
     this.bucketNamePref = bucketNamePref;
@@ -357,16 +358,24 @@ class RemoteSettingsClient extends Event
    * @param {Object} options           additional advanced options.
    * @param {bool}   options.loadDump  load initial dump from disk on first sync (default: true)
    * @param {string} options.trigger   label to identify what triggered this sync (eg. ``"timer"``, default: `"manual"`)
    * @return {Promise}                 which rejects on sync or process failure.
    */
   async maybeSync(expectedTimestamp, options = {}) {
     const { loadDump = true, trigger = "manual" } = options;
 
+    // Make sure we don't run several synchronizations in parallel, mainly
+    // in order to avoid race conditions in "sync" events listeners.
+    if (this._syncRunning) {
+      console.warn(`${this.identifier} sync already running`);
+      return;
+    }
+    this._syncRunning = true;
+
     let importedFromDump = [];
     const startedAt = new Date();
     let reportStatus = null;
     try {
       // Synchronize remote data into a local DB using Kinto.
       const kintoCollection = await this.openCollection();
       let collectionLastModified = await kintoCollection.db.getLastModified();
 
@@ -583,16 +592,17 @@ class RemoteSettingsClient extends Event
       }
       // Report success/error status to Telemetry.
       await UptakeTelemetry.report(TELEMETRY_COMPONENT, reportStatus, {
         source: this.identifier,
         trigger,
         duration: durationMilliseconds,
       });
       console.debug(`${this.identifier} sync status is ${reportStatus}`);
+      this._syncRunning = false;
     }
   }
 
   /**
    * Fetch the signature info from the collection metadata and verifies that the
    * local set of records has the same.
    *
    * @param {Array<Object>} remoteRecords   The list of changes to apply to the local database.
--- a/services/settings/test/unit/test_remote_settings.js
+++ b/services/settings/test/unit/test_remote_settings.js
@@ -346,16 +346,30 @@ add_task(async function test_get_does_no
   // If metadata is missing locally, it is fetched by default (`syncIfEmpty: true`)
   await clientWithDump.get({ verifySignature: true });
   const metadata = await (await clientWithDump.openCollection()).metadata();
   ok(Object.keys(metadata).length > 0, "metadata was fetched");
   ok(called, "signature was verified for the data that was in dump");
 });
 add_task(clear_state);
 
+add_task(async function test_sync_runs_once_only() {
+  const backup = Utils.log.warn;
+  const messages = [];
+  Utils.log.warn = (m) => {
+    messages.push(m);
+  };
+
+  await Promise.all([client.maybeSync(2000), client.maybeSync(2000)]);
+
+  ok(messages.includes("main/password-fields sync already running"), "warning is shown about sync already running");
+  Utils.log.warn = backup;
+});
+add_task(clear_state);
+
 add_task(
   async function test_sync_pulls_metadata_if_missing_with_dump_is_up_to_date() {
     if (IS_ANDROID) {
       // Skip test: we don't ship remote settings dumps on Android (see package-manifest).
       return;
     }
 
     let called;