Bug 1460525 - Execute every Remote Settings event listeners when one fails r=mgoodwin

MozReview-Commit-ID: KOqESGbzapC

--HG--
extra : rebase_source : 997be7ca098703044716db5553f34a55163c8384

services/common/docs/RemoteSettings.rst

@@ -68,9 +68,6 @@ The ``sync`` event allows to be notified when the remote settings are changed on
       }
     });
 
-.. 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).
 

services/common/remote-settings.js

@@ -188,8 +188,8 @@ class RemoteSettingsClient {
     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;
   }
@@ -208,11 +208,36 @@ class RemoteSettingsClient {
     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);
   }
 
   /**
@@ -409,14 +434,9 @@ class RemoteSettingsClient {
         // 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;

services/common/tests/unit/test_remote_settings.js

@@ -16,6 +16,8 @@ async function clear_state() {
   // Clear local DB.
   const collection = await client.openCollection();
   await collection.clear();
+  // Reset event listeners.
+  client._listeners.set("sync", []);
 }
 
 
@@ -141,6 +143,26 @@ add_task(async function test_sync_event_provides_information_about_records() {
 });
 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();