Bug 1460525 - Execute every Remote Settings event listeners when one fails r=mgoodwin
authorMathieu Leplatre <mathieu@mozilla.com>
Mon, 28 May 2018 16:10:27 +0200
changeset 420237 09582e9a65939bc59f99596053575af85160db08
parent 420236 d25c080212f0dd7b19d7166e145555a27a71c0e7
child 420238 56bf1eeea50a4dfb26864fc983ef32ea410d1147
push id34068
push usernerli@mozilla.com
push dateTue, 29 May 2018 21:40:19 +0000
treeherdermozilla-central@5852258260e1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmgoodwin
bugs1460525
milestone62.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 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.