Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects. r=Paolo draft
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 24 Jan 2017 17:38:35 -0800
changeset 466218 ad08e72ac3812f3ad6169c35161bd46eddd32a65
parent 464990 5a4412474c63e1d9e66036d603ac42e9cb2b9150
child 466219 a8a2ee2a02b7118a55eb9e9c9da66f904f67cbf0
push id42841
push userbmo:kit@mozilla.com
push dateWed, 25 Jan 2017 16:45:00 +0000
reviewersPaolo
bugs1332024
milestone54.0a1
Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects. r=Paolo MozReview-Commit-ID: QaRqLBGEft
toolkit/modules/JSONFile.jsm
toolkit/modules/tests/xpcshell/test_JSONFile.js
--- a/toolkit/modules/JSONFile.jsm
+++ b/toolkit/modules/JSONFile.jsm
@@ -81,34 +81,40 @@ const kSaveDelayMs = 1500;
  *        - saveDelayMs: Number indicating the delay (in milliseconds) between a
  *                       change to the data and the related save operation. The
  *                       default value will be applied if omitted.
  *        - beforeSave: Promise-returning function triggered just before the
  *                      data is written to disk. This can be used to create any
  *                      intermediate directories before saving. The file will
  *                      not be saved if the promise rejects or the function
  *                      throws an exception.
+ *        - finalizeAt: An `AsyncShutdown` phase or barrier client that should
+ *                      automatically finalize the file when triggered. Defaults
+ *                      to `profileBeforeChange`; exposed as an option for
+ *                      testing.
  */
 function JSONFile(config) {
   this.path = config.path;
 
   if (typeof config.dataPostProcessor === "function") {
     this._dataPostProcessor = config.dataPostProcessor;
   }
   if (typeof config.beforeSave === "function") {
     this._beforeSave = config.beforeSave;
   }
 
   if (config.saveDelayMs === undefined) {
     config.saveDelayMs = kSaveDelayMs;
   }
   this._saver = new DeferredTask(() => this._save(), config.saveDelayMs);
 
-  AsyncShutdown.profileBeforeChange.addBlocker("JSON store: writing data",
-                                               () => this._saver.finalize());
+  this._finalizeAt = config.finalizeAt || AsyncShutdown.profileBeforeChange;
+  this._finalizeInternalBound = this._finalizeInternal.bind(this);
+  this._finalizeAt.addBlocker("JSON store: writing data",
+                              this._finalizeInternalBound);
 }
 
 JSONFile.prototype = {
   /**
    * String containing the file path where data should be saved.
    */
   path: "",
 
@@ -123,16 +129,23 @@ JSONFile.prototype = {
   _saver: null,
 
   /**
    * Internal data object.
    */
   _data: null,
 
   /**
+   * Internal fields used during finalization.
+   */
+  _finalizeAt: null,
+  _finalizePromise: null,
+  _finalizeInternalBound: null,
+
+  /**
    * Serializable object containing the data. This is populated directly with
    * the data loaded from the file, and is saved without modifications.
    *
    * The raw data should be manipulated synchronously, without waiting for the
    * event loop or for promise resolution, so that the saved file is always
    * consistent.
    */
   get data() {
@@ -275,11 +288,50 @@ JSONFile.prototype = {
     yield OS.File.writeAtomic(this.path, bytes,
                               { tmpPath: this.path + ".tmp" });
   }),
 
   /**
    * Synchronously work on the data just loaded into memory.
    */
   _processLoadedData(data) {
+    if (this._finalizePromise) {
+      // It's possible for `load` to race with `finalize`. In that case, don't
+      // process or set the loaded data.
+      return;
+    }
     this.data = this._dataPostProcessor ? this._dataPostProcessor(data) : data;
   },
+
+  /**
+   * Finishes persisting data to disk and resets all state for this file.
+   *
+   * @return {Promise}
+   * @resolves When the object is finalized.
+   */
+  _finalizeInternal() {
+    if (this._finalizePromise) {
+      // Finalization already in progress; return the pending promise. This is
+      // possible if `finalize` is called concurrently with shutdown.
+      return this._finalizePromise;
+    }
+    this._finalizePromise = Task.spawn(function* () {
+      yield this._saver.finalize();
+      this._data = null;
+      this.dataReady = false;
+    }.bind(this));
+    return this._finalizePromise;
+  },
+
+  /**
+   * Ensures that all data is persisted to disk, and prevents future calls to
+   * `saveSoon`. This is called automatically on shutdown, but can also be
+   * called explicitly when the file is no longer needed.
+   */
+  finalize: Task.async(function* () {
+    if (this._finalizePromise) {
+      throw new Error(`The file ${this.path} has already been finalized`);
+    }
+    // Wait for finalization before removing the shutdown blocker.
+    yield this._finalizeInternal();
+    this._finalizeAt.removeBlocker(this._finalizeInternalBound);
+  }),
 };
--- a/toolkit/modules/tests/xpcshell/test_JSONFile.js
+++ b/toolkit/modules/tests/xpcshell/test_JSONFile.js
@@ -5,16 +5,18 @@
 "use strict";
 
 // Globals
 
 const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
+XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
+                                  "resource://gre/modules/AsyncShutdown.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "DownloadPaths",
                                   "resource://gre/modules/DownloadPaths.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
                                   "resource://gre/modules/FileUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "JSONFile",
                                   "resource://gre/modules/JSONFile.jsm");
@@ -296,8 +298,63 @@ add_task(function* test_beforeSave_rejec
     };
     storeForSave.saveSoon();
   });
 
   yield Assert.rejects(promiseSave, function(ex) {
     return ex.message == "oops";
   });
 });
+
+add_task(function* test_finalize() {
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+
+  let barrier = new AsyncShutdown.Barrier("test-auto-finalize");
+  let storeForSave = new JSONFile({
+    path,
+    saveDelayMs: 2000,
+    finalizeAt: barrier.client,
+  });
+  yield storeForSave.load();
+  storeForSave.data = TEST_DATA;
+  storeForSave.saveSoon();
+
+  let promiseFinalize = storeForSave.finalize();
+  yield Assert.rejects(storeForSave.finalize(), /has already been finalized$/);
+  yield promiseFinalize;
+  do_check_false(storeForSave.dataReady);
+
+  // Finalization removes the blocker, so waiting should not log an unhandled
+  // error even though the object has been explicitly finalized.
+  yield barrier.wait();
+
+  let storeForLoad = new JSONFile({ path });
+  yield storeForLoad.load();
+  do_check_matches(storeForLoad.data, TEST_DATA);
+});
+
+add_task(function* test_finalize_on_shutdown() {
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+
+  let barrier = new AsyncShutdown.Barrier("test-finalize-shutdown");
+  let storeForSave = new JSONFile({
+    path,
+    saveDelayMs: 2000,
+    finalizeAt: barrier.client,
+  });
+  yield storeForSave.load();
+  storeForSave.data = TEST_DATA;
+  // Arm the saver, then simulate shutdown and ensure the file is
+  // automatically finalized.
+  storeForSave.saveSoon();
+
+  yield barrier.wait();
+  // It's possible for `finalize` to reject when called concurrently with
+  // shutdown. We don't distinguish between explicit `finalize` calls and
+  // finalization on shutdown because we expect most consumers to rely on the
+  // latter. However, this behavior can be safely changed if needed.
+  yield Assert.rejects(storeForSave.finalize(), /has already been finalized$/);
+  do_check_false(storeForSave.dataReady);
+
+  let storeForLoad = new JSONFile({ path });
+  yield storeForLoad.load();
+  do_check_matches(storeForLoad.data, TEST_DATA);
+});