Bug 855024 - Better management of pull-only providers; r=rnewman, a=bajaj
authorGregory Szorc <gps@mozilla.com>
Tue, 16 Apr 2013 11:46:05 -0700
changeset 137411 e8e3419872ac20e6d3ce8e5c9c2bba9fe28a5544
parent 137410 29961e1f97e6e20b7e340011cacc898c7cc2273e
child 137412 b8aeaed19ddd275cd8ddb4ba8cebb0e178a89a32
push id2452
push userlsblakk@mozilla.com
push dateMon, 13 May 2013 16:59:38 +0000
treeherdermozilla-beta@d4b152d29d8d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, bajaj
bugs855024
milestone22.0a2
Bug 855024 - Better management of pull-only providers; r=rnewman, a=bajaj
services/healthreport/tests/xpcshell/test_healthreporter.js
services/metrics/providermanager.jsm
services/metrics/tests/xpcshell/test_metrics_provider_manager.js
--- a/services/healthreport/tests/xpcshell/test_healthreporter.js
+++ b/services/healthreport/tests/xpcshell/test_healthreporter.js
@@ -193,16 +193,18 @@ add_task(function test_pull_only_provide
     yield reporter._providerManager.registerProvidersFromCategoryManager(category);
     do_check_eq(reporter._providerManager._providers.size, 1);
     do_check_true(reporter._storage.hasProvider("DummyProvider"));
     do_check_false(reporter._storage.hasProvider("DummyConstantProvider"));
     do_check_neq(reporter.getProvider("DummyProvider"), null);
     do_check_null(reporter.getProvider("DummyConstantProvider"));
 
     yield reporter._providerManager.ensurePullOnlyProvidersRegistered();
+    do_check_eq(reporter._providerManager._providers.size, 2);
+    do_check_true(reporter._storage.hasProvider("DummyConstantProvider"));
     yield reporter.collectMeasurements();
     yield reporter._providerManager.ensurePullOnlyProvidersUnregistered();
 
     do_check_eq(reporter._providerManager._providers.size, 1);
     do_check_true(reporter._storage.hasProvider("DummyConstantProvider"));
 
     let mID = reporter._storage.measurementID("DummyConstantProvider", "DummyMeasurement", 1);
     let values = yield reporter._storage.getMeasurementValues(mID);
--- a/services/metrics/providermanager.jsm
+++ b/services/metrics/providermanager.jsm
@@ -29,24 +29,31 @@ this.ProviderManager = function (storage
 
   this._providers = new Map();
   this._storage = storage;
 
   this._providerInitQueue = [];
   this._providerInitializing = false;
 
   this._pullOnlyProviders = {};
-  this._pullOnlyProvidersRegistered = false;
+  this._pullOnlyProvidersRegisterCount = 0;
+  this._pullOnlyProvidersState = this.PULL_ONLY_NOT_REGISTERED;
+  this._pullOnlyProvidersCurrentPromise = null;
 
   // Callback to allow customization of providers after they are constructed
   // but before they call out into their initialization code.
   this.onProviderInit = null;
 }
 
 this.ProviderManager.prototype = Object.freeze({
+  PULL_ONLY_NOT_REGISTERED: "none",
+  PULL_ONLY_REGISTERING: "registering",
+  PULL_ONLY_UNREGISTERING: "unregistering",
+  PULL_ONLY_REGISTERED: "registered",
+
   get providers() {
     let providers = [];
     for (let [name, entry] of this._providers) {
       providers.push(entry.provider);
     }
 
     return providers;
   },
@@ -220,68 +227,167 @@ this.ProviderManager.prototype = Object.
   unregisterProvider: function (name) {
     this._providers.delete(name);
   },
 
   /**
    * Ensure that pull-only providers are registered.
    */
   ensurePullOnlyProvidersRegistered: function () {
-    if (this._pullOnlyProvidersRegistered) {
+    let state = this._pullOnlyProvidersState;
+
+    this._pullOnlyProvidersRegisterCount++;
+
+    if (state == this.PULL_ONLY_REGISTERED) {
+      this._log.debug("Requested pull-only provider registration and " +
+                      "providers are already registered.");
       return CommonUtils.laterTickResolvingPromise();
     }
 
-    let onFinished = function () {
-      this._pullOnlyProvidersRegistered = true;
+    // If we're in the process of registering, chain off that request.
+    if (state == this.PULL_ONLY_REGISTERING) {
+      this._log.debug("Requested pull-only provider registration and " +
+                      "registration is already in progress.");
+      return this._pullOnlyProvidersCurrentPromise;
+    }
+
+    this._log.debug("Pull-only provider registration requested.");
+
+    // A side-effect of setting this is that an active unregistration will
+    // effectively short circuit and finish as soon as the in-flight
+    // unregistration (if any) finishes.
+    this._pullOnlyProvidersState = this.PULL_ONLY_REGISTERING;
+
+    let inFlightPromise = this._pullOnlyProvidersCurrentPromise;
+
+    this._pullOnlyProvidersCurrentPromise =
+      Task.spawn(function registerPullProviders() {
 
-      return CommonUtils.laterTickResolvingPromise();
-    }.bind(this);
+      if (inFlightPromise) {
+        this._log.debug("Waiting for in-flight pull-only provider activity " +
+                        "to finish before registering.");
+        try {
+          yield inFlightPromise;
+        } catch (ex) {
+          this._log.warn("Error when waiting for existing pull-only promise: " +
+                         CommonUtils.exceptionStr(ex));
+        }
+      }
 
-    return Task.spawn(function registerPullProviders() {
       for each (let providerType in this._pullOnlyProviders) {
+        // Short-circuit if we're no longer registering.
+        if (this._pullOnlyProvidersState != this.PULL_ONLY_REGISTERING) {
+          this._log.debug("Aborting pull-only provider registration.");
+          break;
+        }
+
         try {
           let provider = this._initProviderFromType(providerType);
+
+          // This is a no-op if the provider is already registered. So, the
+          // only overhead is constructing an instance. This should be cheap
+          // and isn't worth optimizing.
           yield this.registerProvider(provider);
         } catch (ex) {
           this._recordError("Error registering pull-only provider", ex);
         }
       }
-    }.bind(this)).then(onFinished, onFinished);
+
+      // It's possible we changed state while registering. Only mark as
+      // registered if we didn't change state.
+      if (this._pullOnlyProvidersState == this.PULL_ONLY_REGISTERING) {
+        this._pullOnlyProvidersState = this.PULL_ONLY_REGISTERED;
+        this._pullOnlyProvidersCurrentPromise = null;
+      }
+    }.bind(this));
+    return this._pullOnlyProvidersCurrentPromise;
   },
 
   ensurePullOnlyProvidersUnregistered: function () {
-    if (!this._pullOnlyProvidersRegistered) {
+    let state = this._pullOnlyProvidersState;
+
+    // If we're not registered, this is a no-op.
+    if (state == this.PULL_ONLY_NOT_REGISTERED) {
+      this._log.debug("Requested pull-only provider unregistration but none " +
+                      "are registered.");
       return CommonUtils.laterTickResolvingPromise();
     }
 
-    let onFinished = function () {
-      this._pullOnlyProvidersRegistered = false;
+    // If we're currently unregistering, recycle the promise from last time.
+    if (state == this.PULL_ONLY_UNREGISTERING) {
+      this._log.debug("Requested pull-only provider unregistration and " +
+                 "unregistration is in progress.");
+      this._pullOnlyProvidersRegisterCount =
+        Math.max(0, this._pullOnlyProvidersRegisterCount - 1);
 
+      return this._pullOnlyProvidersCurrentPromise;
+    }
+
+    // We ignore this request while multiple entities have requested
+    // registration because we don't want a request from an "inner,"
+    // short-lived request to overwrite the desire of the "parent,"
+    // longer-lived request.
+    if (this._pullOnlyProvidersRegisterCount > 1) {
+      this._log.debug("Requested pull-only provider unregistration while " +
+                      "other callers still want them registered. Ignoring.");
+      this._pullOnlyProvidersRegisterCount--;
       return CommonUtils.laterTickResolvingPromise();
-    }.bind(this);
+    }
+
+    // We are either fully registered or registering with a single consumer.
+    // In both cases we are authoritative and can commence unregistration.
+
+    this._log.debug("Pull-only providers being unregistered.");
+    this._pullOnlyProvidersRegisterCount =
+      Math.max(0, this._pullOnlyProvidersRegisterCount - 1);
+    this._pullOnlyProvidersState = this.PULL_ONLY_UNREGISTERING;
+    let inFlightPromise = this._pullOnlyProvidersCurrentPromise;
+
+    this._pullOnlyProvidersCurrentPromise =
+      Task.spawn(function unregisterPullProviders() {
 
-    return Task.spawn(function unregisterPullProviders() {
+      if (inFlightPromise) {
+        this._log.debug("Waiting for in-flight pull-only provider activity " +
+                        "to complete before unregistering.");
+        try {
+          yield inFlightPromise;
+        } catch (ex) {
+          this._log.warn("Error when waiting for existing pull-only promise: " +
+                         CommonUtils.exceptionStr(ex));
+        }
+      }
+
       for (let provider of this.providers) {
+        if (this._pullOnlyProvidersState != this.PULL_ONLY_UNREGISTERING) {
+          return;
+        }
+
         if (!provider.pullOnly) {
           continue;
         }
 
         this._log.info("Shutting down pull-only provider: " +
                        provider.name);
 
         try {
           yield provider.shutdown();
         } catch (ex) {
           this._recordError("Error when shutting down provider: " +
                             provider.name, ex);
         } finally {
           this.unregisterProvider(provider.name);
         }
       }
-    }.bind(this)).then(onFinished, onFinished);
+
+      if (this._pullOnlyProvidersState == this.PULL_ONLY_UNREGISTERING) {
+        this._pullOnlyProvidersState = this.PULL_ONLY_NOT_REGISTERED;
+        this._pullOnlyProvidersCurrentPromise = null;
+      }
+    }.bind(this));
+    return this._pullOnlyProvidersCurrentPromise;
   },
 
   _popAndInitProvider: function () {
     if (!this._providerInitQueue.length || this._providerInitializing) {
       return;
     }
 
     let [provider, deferred] = this._providerInitQueue.shift();
--- a/services/metrics/tests/xpcshell/test_metrics_provider_manager.js
+++ b/services/metrics/tests/xpcshell/test_metrics_provider_manager.js
@@ -3,17 +3,28 @@
 
 "use strict";
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://gre/modules/Metrics.jsm");
 Cu.import("resource://testing-common/services/metrics/mocks.jsm");
 
+const PULL_ONLY_TESTING_CATEGORY = "testing-only-pull-only-providers";
+
 function run_test() {
+  let cm = Cc["@mozilla.org/categorymanager;1"]
+             .getService(Ci.nsICategoryManager);
+  cm.addCategoryEntry(PULL_ONLY_TESTING_CATEGORY, "DummyProvider",
+                      "resource://testing-common/services/metrics/mocks.jsm",
+                      false, true);
+  cm.addCategoryEntry(PULL_ONLY_TESTING_CATEGORY, "DummyConstantProvider",
+                      "resource://testing-common/services/metrics/mocks.jsm",
+                      false, true);
+
   run_next_test();
 };
 
 add_task(function test_constructor() {
   let storage = yield Metrics.Storage("constructor");
   let manager = new Metrics.ProviderManager(storage);
 
   yield storage.close();
@@ -170,8 +181,90 @@ add_task(function test_collect_daily() {
 
   yield manager.collectDailyData();
   do_check_eq(provider1.collectDailyCount, 2);
   do_check_eq(provider2.collectDailyCount, 2);
 
   yield storage.close();
 });
 
+add_task(function test_pull_only_not_initialized() {
+  let storage = yield Metrics.Storage("pull_only_not_initialized");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+  do_check_eq(manager.providers.length, 1);
+  do_check_eq(manager.providers[0].name, "DummyProvider");
+  yield storage.close();
+});
+
+add_task(function test_pull_only_registration() {
+  let storage = yield Metrics.Storage("pull_only_registration");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+  do_check_eq(manager.providers.length, 1);
+
+  // Simple registration and unregistration.
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+  do_check_neq(manager.getProvider("DummyConstantProvider"), null);
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+  do_check_null(manager.getProvider("DummyConstantProvider"));
+
+  // Multiple calls to register work.
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+
+  // Unregister with 2 requests for registration should not unregister.
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 2);
+
+  // But the 2nd one will.
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+
+  yield storage.close();
+});
+
+add_task(function test_pull_only_register_while_registering() {
+  let storage = yield Metrics.Storage("pull_only_register_will_registering");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+
+  manager.ensurePullOnlyProvidersRegistered();
+  manager.ensurePullOnlyProvidersRegistered();
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+
+  manager.ensurePullOnlyProvidersUnregistered();
+  manager.ensurePullOnlyProvidersUnregistered();
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+
+  yield storage.close();
+});
+
+add_task(function test_pull_only_unregister_while_registering() {
+  let storage = yield Metrics.Storage("pull_only_unregister_while_registering");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+
+  manager.ensurePullOnlyProvidersRegistered();
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+
+  yield storage.close();
+});
+
+add_task(function test_pull_only_register_while_unregistering() {
+  let storage = yield Metrics.Storage("pull_only_register_while_unregistering");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+
+  yield manager.ensurePullOnlyProvidersRegistered();
+  manager.ensurePullOnlyProvidersUnregistered();
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+
+  yield storage.close();
+});