Bug 1460525 - Execute every Remote Settings event listeners when one fails r?mgoodwin draft
authorMathieu Leplatre <mathieu@mozilla.com>
Mon, 28 May 2018 16:10:27 +0200
changeset 800573 f71543c1b7bbca816a9dfa5225c335a2c8ac735b
parent 800491 a466172aed4bc2afc21169b749b8068a4b98c93f
push id111403
push usermleplatre@mozilla.com
push dateMon, 28 May 2018 14:10:53 +0000
reviewersmgoodwin
bugs1460525
milestone62.0a1
Bug 1460525 - Execute every Remote Settings event listeners when one fails r?mgoodwin MozReview-Commit-ID: KOqESGbzapC
services/common/docs/RemoteSettings.rst
services/common/remote-settings.js
services/common/tests/unit/test_remote_settings.js
--- a/services/common/docs/RemoteSettings.rst
+++ b/services/common/docs/RemoteSettings.rst
@@ -63,19 +63,16 @@ The ``sync`` event allows to be notified
     RemoteSettings("a-key").on("sync", event => {
       const { data: { current } } = event;
       for(const entry of current) {
         // Do something with entry...
         // await InternalAPI.reload(entry.id, entry.label, entry.weight);
       }
     });
 
-.. important::
-    If one of the event handler fails, the others handlers for the same remote settings collection won't be executed.
-
 .. note::
     Currently, the update of remote settings is triggered by the `nsBlocklistService <https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js>`_ (~ every 24H).
 
 File attachments
 ----------------
 
 When an entry has a file attached to it, it has an ``attachment`` attribute, which contains the file related information (url, hash, size, mimetype, etc.). Remote files are not downloaded automatically.
 
--- a/services/common/remote-settings.js
+++ b/services/common/remote-settings.js
@@ -183,18 +183,18 @@ class RemoteSettingsClient {
 
   constructor(collectionName, { bucketName, signerName, filterFunc = jexlFilterFunc, lastCheckTimePref }) {
     this.collectionName = collectionName;
     this.bucketName = bucketName;
     this.signerName = signerName;
     this.filterFunc = filterFunc;
     this._lastCheckTimePref = lastCheckTimePref;
 
-    this._callbacks = new Map();
-    this._callbacks.set("sync", []);
+    this._listeners = new Map();
+    this._listeners.set("sync", []);
 
     this._kinto = null;
   }
 
   get identifier() {
     return `${this.bucketName}/${this.collectionName}`;
   }
 
@@ -203,21 +203,46 @@ class RemoteSettingsClient {
     const identifier = OS.Path.join(...this.identifier.split("/"));
     return `${identifier}.json`;
   }
 
   get lastCheckTimePref() {
     return this._lastCheckTimePref || `services.settings.${this.bucketName}.${this.collectionName}.last_check`;
   }
 
+  /**
+   * Event emitter: will execute the registered listeners in the order and
+   * sequentially.
+   *
+   * Note: we don't use `toolkit/modules/EventEmitter` because we want to throw
+   * an error when a listener fails to execute.
+   *
+   * @param {string} event    the event name
+   * @param {Object} payload  the event payload to call the listeners with
+   */
+  async emit(event, payload) {
+    const callbacks = this._listeners.get("sync");
+    let firstError;
+    for (const cb of callbacks) {
+      try {
+        await cb(payload);
+      } catch (e) {
+        firstError = e;
+      }
+    }
+    if (firstError) {
+      throw firstError;
+    }
+  }
+
   on(event, callback) {
-    if (!this._callbacks.has(event)) {
+    if (!this._listeners.has(event)) {
       throw new Error(`Unknown event type ${event}`);
     }
-    this._callbacks.get(event).push(callback);
+    this._listeners.get(event).push(callback);
   }
 
   /**
    * Open the underlying Kinto collection, using the appropriate adapter and
    * options. This acts as a context manager where the connection is closed
    * once the specified `callback` has finished.
    *
    * @param {callback} function           the async function to execute with the open SQlite connection.
@@ -404,24 +429,19 @@ class RemoteSettingsClient {
       const updatedFilteredIds = new Set(updatedFiltered.map(e => e.id));
       const updated = allUpdated.filter(({ new: { id } }) => updatedFilteredIds.has(id));
 
       // If every changed entry is filtered, we don't even fire the event.
       if (created.length || updated.length || deleted.length) {
         // Read local collection of records (also filtered).
         const { data: allData } = await collection.list();
         const current = await this._filterEntries(allData);
-        // Fire the event: execute callbacks in order and sequentially.
-        // If one fails everything fails.
-        const event = { data: { current, created, updated, deleted } };
-        const callbacks = this._callbacks.get("sync");
+        const payload = { data: { current, created, updated, deleted } };
         try {
-          for (const cb of callbacks) {
-            await cb(event);
-          }
+          await this.emit("sync", payload);
         } catch (e) {
           reportStatus = UptakeTelemetry.STATUS.APPLY_ERROR;
           throw e;
         }
       }
 
       // Track last update.
       this._updateLastCheck(serverTime);
--- a/services/common/tests/unit/test_remote_settings.js
+++ b/services/common/tests/unit/test_remote_settings.js
@@ -11,16 +11,18 @@ const BinaryInputStream = CC("@mozilla.o
 
 let server;
 let client;
 
 async function clear_state() {
   // Clear local DB.
   const collection = await client.openCollection();
   await collection.clear();
+  // Reset event listeners.
+  client._listeners.set("sync", []);
 }
 
 
 function run_test() {
   // Set up an HTTP Server
   server = new HttpServer();
   server.start(-1);
 
@@ -136,16 +138,36 @@ add_task(async function test_sync_event_
   equal(eventData.current.length, 1);
   equal(eventData.created.length, 0);
   equal(eventData.updated.length, 0);
   equal(eventData.deleted.length, 1);
   equal(eventData.deleted[0].website, "https://www.other.org/signin");
 });
 add_task(clear_state);
 
+add_task(async function test_all_listeners_are_executed_if_one_fails() {
+  const serverTime = Date.now();
+
+  let count = 0;
+  client.on("sync", () => { count += 1; });
+  client.on("sync", () => { throw new Error("boom"); });
+  client.on("sync", () => { count += 2; });
+
+  let error;
+  try {
+    await client.maybeSync(2000, serverTime);
+  } catch (e) {
+    error = e;
+  }
+
+  equal(count, 3);
+  equal(error.message, "boom");
+});
+add_task(clear_state);
+
 add_task(async function test_telemetry_reports_up_to_date() {
   await client.maybeSync(2000, Date.now() - 1000);
   const serverTime = Date.now();
   const startHistogram = getUptakeTelemetrySnapshot(client.identifier);
 
   await client.maybeSync(3000, serverTime);
 
   // No Telemetry was sent.