Bug 1385440 - Serialize client commands additions. r=markh, a=lizzard
authorEdouard Oger <eoger@fastmail.com>
Wed, 02 Aug 2017 11:33:24 -0400
changeset 423451 bb925de1a0f30951c0b3d028716cc9ee69ac0676
parent 423450 e648c321f8d348dc6dc7acddce8b10a257aab8c4
child 423452 6f5f957aa6dd36c0b4c51c3bd943ef3c9c5bdece
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, lizzard
bugs1385440
milestone56.0
Bug 1385440 - Serialize client commands additions. r=markh, a=lizzard MozReview-Commit-ID: 3qGcHVhOCeL
services/sync/modules/engines/clients.js
services/sync/tests/unit/test_clients_engine.js
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -77,16 +77,17 @@ Utils.deferGetSet(ClientsRec,
 
 
 this.ClientEngine = function ClientEngine(service) {
   SyncEngine.call(this, "Clients", service);
 
   // Reset the last sync timestamp on every startup so that we fetch all clients
   this.resetLastSync();
   this.fxAccounts = fxAccounts;
+  this.addClientCommandQueue = Promise.resolve();
 }
 ClientEngine.prototype = {
   __proto__: SyncEngine.prototype,
   _storeObj: ClientStore,
   _recordObj: ClientsRec,
   _trackerObj: ClientsTracker,
   allowSkippedRecord: false,
   _knownStaleFxADeviceIds: null,
@@ -270,30 +271,39 @@ ClientEngine.prototype = {
 
   async removeLocalCommand(command) {
     // the implementation of this engine is such that adding a command to
     // the local client is how commands are deleted! ¯\_(ツ)_/¯
     await this._addClientCommand(this.localID, command);
   },
 
   async _addClientCommand(clientId, command) {
-    const localCommands = await this._readCommands();
-    const localClientCommands = localCommands[clientId] || [];
-    const remoteClient = this._store._remoteClients[clientId];
-    let remoteClientCommands = []
-    if (remoteClient && remoteClient.commands) {
-      remoteClientCommands = remoteClient.commands;
-    }
-    const clientCommands = localClientCommands.concat(remoteClientCommands);
-    if (hasDupeCommand(clientCommands, command)) {
-      return false;
-    }
-    localCommands[clientId] = localClientCommands.concat(command);
-    await this._saveCommands(localCommands);
-    return true;
+    return this.addClientCommandQueue = (async () => {
+      await this.addClientCommandQueue;
+      try {
+        const localCommands = await this._readCommands();
+        const localClientCommands = localCommands[clientId] || [];
+        const remoteClient = this._store._remoteClients[clientId];
+        let remoteClientCommands = []
+        if (remoteClient && remoteClient.commands) {
+          remoteClientCommands = remoteClient.commands;
+        }
+        const clientCommands = localClientCommands.concat(remoteClientCommands);
+        if (hasDupeCommand(clientCommands, command)) {
+          return false;
+        }
+        localCommands[clientId] = localClientCommands.concat(command);
+        await this._saveCommands(localCommands);
+        return true;
+      } catch (e) {
+        // Failing to save a command should not "break the queue" of pending operations.
+        this._log.error(e);
+        return false;
+      }
+    })();
   },
 
   async _removeClientCommands(clientId) {
     const allCommands = await this._readCommands();
     delete allCommands[clientId];
     await this._saveCommands(allCommands);
   },
 
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -431,16 +431,31 @@ add_task(async function test_send_comman
   deepEqual(command.args, args);
   ok(command.flowID);
 
   notEqual(tracker.changedIDs[remoteId], undefined);
 
   await cleanup();
 });
 
+// The browser UI might call _addClientCommand indirectly without awaiting on the returned promise.
+// We need to make sure this doesn't result on commands not being saved.
+add_task(async function test_add_client_command_race() {
+  let promises = [];
+  for (let i = 0; i < 100; i++) {
+    promises.push(engine._addClientCommand(`client-${i}`, { command: "cmd", args: []}));
+  }
+  await Promise.all(promises);
+
+  let localCommands = await engine._readCommands();
+  for (let i = 0; i < 100; i++) {
+    equal(localCommands[`client-${i}`].length, 1);
+  }
+});
+
 add_task(async function test_command_validation() {
   _("Verifies that command validation works properly.");
 
   let store = engine._store;
 
   let testCommands = [
     ["resetAll",    [],       true ],
     ["resetAll",    ["foo"],  false],