Bug 1635408 - abort non-critical RemoteSettingsWorker tasks at shutdown, r=leplatrem
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 05 May 2020 23:52:17 +0000
changeset 528311 5db356e1bb9414fff740f81c03c07e9a2dfb3561
parent 528310 320b571a080134bb5010c4a388e6f87a7a633306
child 528312 1aa55ae9da78036003e1f472cee4d50b00740e0d
push id37383
push userrgurzau@mozilla.com
push dateWed, 06 May 2020 09:37:16 +0000
treeherdermozilla-central@1fa1d4f4b0e2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersleplatrem
bugs1635408
milestone78.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 1635408 - abort non-critical RemoteSettingsWorker tasks at shutdown, r=leplatrem Differential Revision: https://phabricator.services.mozilla.com/D73858
services/settings/RemoteSettingsWorker.js
services/settings/RemoteSettingsWorker.jsm
services/settings/test/unit/test_shutdown_handling.js
--- a/services/settings/RemoteSettingsWorker.js
+++ b/services/settings/RemoteSettingsWorker.js
@@ -79,18 +79,17 @@ const Agent = {
     let resp;
     try {
       resp = await fetch(fileUrl);
     } catch (e) {
       // File does not exist.
       return false;
     }
     const buffer = await resp.arrayBuffer();
-    const bytes = new Uint8Array(buffer);
-    return this.checkContentHash(bytes, size, hash);
+    return this.checkContentHash(buffer, size, hash);
   },
 
   /**
    * Check that the specified content matches the expected size and SHA-256 hash.
    * @param {ArrayBuffer} buffer binary content
    * @param {Number} size expected file size
    * @param {String} size expected file SHA-256 as hex string
    * @returns {boolean}
--- a/services/settings/RemoteSettingsWorker.jsm
+++ b/services/settings/RemoteSettingsWorker.jsm
@@ -50,48 +50,55 @@ class Worker {
     this.source = source;
     this.worker = null;
 
     this.callbacks = new Map();
     this.lastCallbackId = 0;
     this.idleTimeoutId = null;
   }
 
-  async _execute(method, args = []) {
+  async _execute(method, args = [], options = {}) {
     if (gShutdown) {
       throw new RemoteSettingsWorkerError("Remote Settings has shut down.");
     }
+    const { mustComplete = false } = options;
+
     // (Re)instantiate the worker if it was terminated.
     if (!this.worker) {
       this.worker = new ChromeWorker(this.source);
       this.worker.onmessage = this._onWorkerMessage.bind(this);
       this.worker.onerror = error => {
         // Worker crashed. Reject each pending callback.
-        for (const [, reject] of this.callbacks.values()) {
+        for (const { reject } of this.callbacks.values()) {
           reject(error);
         }
         this.callbacks.clear();
         // And terminate it.
         this.stop();
       };
     }
     // New activity: reset the idle timer.
     if (this.idleTimeoutId) {
       clearTimeout(this.idleTimeoutId);
     }
+    let identifier = method + "-";
+    // Include the collection details in the importJSONDump case.
+    if (identifier == "importJSONDump") {
+      identifier += `${args[0]}-${args[1]}-`;
+    }
     return new Promise((resolve, reject) => {
-      const callbackId = `${method}-${++this.lastCallbackId}`;
-      this.callbacks.set(callbackId, [resolve, reject]);
+      const callbackId = `${identifier}${++this.lastCallbackId}`;
+      this.callbacks.set(callbackId, { resolve, reject, mustComplete });
       this.worker.postMessage({ callbackId, method, args });
     });
   }
 
   _onWorkerMessage(event) {
     const { callbackId, result, error } = event.data;
-    const [resolve, reject] = this.callbacks.get(callbackId);
+    const { resolve, reject } = this.callbacks.get(callbackId);
     if (error) {
       reject(new RemoteSettingsWorkerError(error));
     } else {
       resolve(result);
     }
     this.callbacks.delete(callbackId);
 
     // Terminate the worker when it's unused for some time.
@@ -105,32 +112,59 @@ class Worker {
       } else {
         this.idleTimeoutId = setTimeout(() => {
           this.stop();
         }, gMaxIdleMilliseconds);
       }
     }
   }
 
+  /**
+   * Called at shutdown to abort anything the worker is doing that isn't
+   * critical.
+   */
+  _abortCancelableRequests() {
+    // End all tasks that we can.
+    const callbackCopy = Array.from(this.callbacks.entries());
+    const error = new Error("Shutdown, aborting read-only worker requests.");
+    for (const [id, { reject, mustComplete }] of callbackCopy) {
+      if (!mustComplete) {
+        this.callbacks.delete(id);
+        reject(error);
+      }
+    }
+    // There might be nothing left now:
+    if (!this.callbacks.size) {
+      this.stop();
+      if (gShutdownResolver) {
+        gShutdownResolver();
+      }
+    }
+    // If there was something left, we'll stop as soon as we get messages from
+    // those tasks, too.
+  }
+
   stop() {
     this.worker.terminate();
     this.worker = null;
     this.idleTimeoutId = null;
   }
 
   async canonicalStringify(localRecords, remoteRecords, timestamp) {
     return this._execute("canonicalStringify", [
       localRecords,
       remoteRecords,
       timestamp,
     ]);
   }
 
   async importJSONDump(bucket, collection) {
-    return this._execute("importJSONDump", [bucket, collection]);
+    return this._execute("importJSONDump", [bucket, collection], {
+      mustComplete: true,
+    });
   }
 
   async checkFileHash(filepath, size, hash) {
     return this._execute("checkFileHash", [filepath, size, hash]);
   }
 
   async checkContentHash(buffer, size, hash) {
     return this._execute("checkContentHash", [buffer, size, hash]);
@@ -151,20 +185,26 @@ try {
       gShutdown = true;
       // Then, if we have no worker or no callbacks, we're done.
       if (
         !RemoteSettingsWorker.worker ||
         !RemoteSettingsWorker.callbacks.size
       ) {
         return null;
       }
-      // Otherwise, return a promise that the worker will resolve.
-      return new Promise(resolve => {
+      // Otherwise, there's something left to do. Set up a promise:
+      let finishedPromise = new Promise(resolve => {
         gShutdownResolver = resolve;
       });
+
+      // Try to cancel most of the work:
+      RemoteSettingsWorker._abortCancelableRequests();
+
+      // Return a promise that the worker will resolve.
+      return finishedPromise;
     },
     {
       fetchState() {
         const remainingCallbacks = RemoteSettingsWorker.callbacks;
         const details = Array.from(remainingCallbacks.keys()).join(", ");
         return `Remaining: ${remainingCallbacks.size} callbacks (${details}).`;
       },
     }
--- a/services/settings/test/unit/test_shutdown_handling.js
+++ b/services/settings/test/unit/test_shutdown_handling.js
@@ -1,16 +1,25 @@
 /* Any copyright is dedicated to the Public Domain.
 http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
+const { AppConstants } = ChromeUtils.import(
+  "resource://gre/modules/AppConstants.jsm"
+);
 const { Database } = ChromeUtils.import(
   "resource://services-settings/Database.jsm"
 );
+const { RemoteSettingsWorker } = ChromeUtils.import(
+  "resource://services-settings/RemoteSettingsWorker.jsm"
+);
+const { RemoteSettingsClient } = ChromeUtils.import(
+  "resource://services-settings/RemoteSettingsClient.jsm"
+);
 
 add_task(async function test_shutdown_abort_after_start() {
   // Start a forever transaction:
   let counter = 0;
   let transactionStarted;
   let startedPromise = new Promise(r => {
     transactionStarted = r;
   });
@@ -77,9 +86,46 @@ add_task(async function test_shutdown_im
   );
 
   let rejection;
   // Wait for the abort
   await promise.catch(e => {
     rejection = e;
   });
   ok(rejection, "Directly aborted promise should also have rejected.");
+  // Now clear the shutdown flag and rejection error:
+  Database._cancelShutdown();
 });
+
+add_task(async function test_shutdown_worker() {
+  // Fallback for android:
+  let importPromise = Promise.resolve();
+  let client;
+  // Android has no dumps, so only run the import if we do have dumps:
+  if (AppConstants.platform != "android") {
+    client = new RemoteSettingsClient("language-dictionaries", {
+      bucketNamePref: "services.settings.default_bucket",
+    });
+    const before = await client.get({ syncIfEmpty: false });
+    Assert.equal(before.length, 0);
+
+    importPromise = RemoteSettingsWorker.importJSONDump(
+      "main",
+      "language-dictionaries"
+    );
+  }
+  let stringifyPromise = RemoteSettingsWorker.canonicalStringify(
+    [],
+    [],
+    Date.now()
+  );
+  RemoteSettingsWorker._abortCancelableRequests();
+  await Assert.rejects(
+    stringifyPromise,
+    /Shutdown/,
+    "Should have aborted the stringify request at shutdown."
+  );
+  await importPromise.catch(e => ok(false, "importing failed!"));
+  if (client) {
+    const after = await client.get();
+    Assert.ok(after.length > 0);
+  }
+});