fix (onReady): Clear ready timeout when instance closed, and other small fixes (optimizely#25)

mjc1283 · web-flow · commit de8546c1bb3d · 2019-04-19T16:25:16.000-07:00
Summary:

Previously, if close was called with pending ready timeouts, those timeouts would remain active. With this change, we clear all pending ready timeouts when close is called.

To guard against Promises returned from onReady remaining permanently pending, in close, we resolve any ready promises associated with pending timeouts with unsuccessful results. 

There are two other small changes in this PR:
- Improve documentation comments for Optimzely onReady and ProjectConfigManager onReady
- Return false from setForcedVariation when no configObj available (was previously returning null)

Test plan:

Updated unit tests, existing unit tests should continue passing
diff --git a/packages/optimizely-sdk/lib/core/project_config/project_config_manager.js b/packages/optimizely-sdk/lib/core/project_config/project_config_manager.js
@@ -285,8 +285,23 @@ ProjectConfigManager.prototype.getConfig = function() {
 };
 
 /**
- * Returns a Promise that resolves when this ProjectConfigManager has a non-null
- * project config object available for the first time.
+ * Returns a Promise that fulfills when this ProjectConfigManager is ready to
+ * use (meaning it has a valid project config object), or has failed to become
+ * ready.
+ *
+ * Failure can be caused by the following:
+ * - At least one of sdkKey or datafile is not provided in the constructor argument
+ * - The provided datafile was invalid
+ * - The datafile provided by the datafile manager was invalid
+ * - The datafile manager failed to fetch a datafile
+ *
+ * The returned Promise is fulfilled with a result object containing these
+ * properties:
+ *    - success (boolean): True if this instance is ready to use with a valid
+ *                         project config object, or false if it failed to
+ *                         become ready
+ *    - reason (string=):  If success is false, this is a string property with
+ *                         an explanatory message.
  * @return {Promise}
  */
 ProjectConfigManager.prototype.onReady = function() {
diff --git a/packages/optimizely-sdk/lib/optimizely/index.js b/packages/optimizely-sdk/lib/optimizely/index.js
@@ -113,6 +113,9 @@ function Optimizely(config) {
     maxQueueSize: config.eventBatchSize || DEFAULT_EVENT_MAX_QUEUE_SIZE,
   });
   this.eventProcessor.start();
+
+  this.__readyTimeouts = {};
+  this.__nextReadyTimeoutId = 0;
 }
 
 /**
@@ -420,7 +423,7 @@ Optimizely.prototype.setForcedVariation = function(experimentKey, userId, variat
 
   var configObj = this.projectConfigManager.getConfig();
   if (!configObj) {
-    return null;
+    return false;
   }
 
   try {
@@ -582,7 +585,7 @@ Optimizely.prototype.isFeatureEnabled = function(featureKey, userId, attributes)
       source: decision.decisionSource,
       sourceInfo: sourceInfo
     };
-    
+
     this.notificationCenter.sendNotifications(
       NOTIFICATION_TYPES.DECISION,
       {
@@ -857,6 +860,12 @@ Optimizely.prototype.close = function() {
     if (this.projectConfigManager) {
       this.projectConfigManager.stop();
     }
+    Object.keys(this.__readyTimeouts).forEach(function(readyTimeoutId) {
+      var readyTimeoutRecord = this.__readyTimeouts[readyTimeoutId];
+      clearTimeout(readyTimeoutRecord.readyTimeout);
+      readyTimeoutRecord.onClose();
+    }.bind(this));
+    this.__readyTimeouts = {};
   } catch (e) {
     this.logger.log(LOG_LEVEL.ERROR, e.message);
     this.errorHandler.handleError(e);
@@ -866,22 +875,26 @@ Optimizely.prototype.close = function() {
 /**
  * Returns a Promise that fulfills when this instance is ready to use (meaning
  * it has a valid datafile), or has failed to become ready within a period of
- * time (configurable by the timeout property of the options argument). If a
- * valid datafile was provided in the constructor, the instance is immediately
- * ready. If an sdkKey was provided, a manager will be used to fetch a datafile,
- * and the returned promise will resolve if that fetch succeeds or fails before
- * the timeout. The default timeout is 30 seconds, which will be used if no
- * timeout is provided in the argument options object.
+ * time (configurable by the timeout property of the options argument), or when
+ * this instance is closed via the close method.
+ *
+ * If a valid datafile was provided in the constructor, the returned Promise is
+ * immediately fulfilled. If an sdkKey was provided, a manager will be used to
+ * fetch  a datafile, and the returned promise will fulfill if that fetch
+ * succeeds or fails before the timeout. The default timeout is 30 seconds,
+ * which will be used if no timeout is provided in the argument options object.
+ *
  * The returned Promise is fulfilled with a result object containing these
  * properties:
  *    - success (boolean): True if this instance is ready to use with a valid
  *                         datafile, or false if this instance failed to become
- *                         ready.
+ *                         ready or was closed prior to becoming ready.
  *    - reason (string=):  If success is false, this is a string property with
  *                         an explanatory message. Failure could be due to
  *                         expiration of the timeout, network errors,
- *                         unsuccessful responses, datafile parse errors, or
- *                         datafile validation errors.
+ *                         unsuccessful responses, datafile parse errors,
+ *                         datafile validation errors, or the instance being
+ *                         closed
  * @param  {Object=}          options
  * @param  {number|undefined} options.timeout
  * @return {Promise}
@@ -894,14 +907,35 @@ Optimizely.prototype.onReady = function(options) {
   if (!fns.isFinite(timeout)) {
     timeout = DEFAULT_ONREADY_TIMEOUT;
   }
+
+  var resolveTimeoutPromise;
   var timeoutPromise = new Promise(function(resolve) {
-    setTimeout(function() {
-      resolve({
-        success: false,
-        reason: sprintf('onReady timeout expired after %s ms', timeout),
-      });
-    }, timeout);
+    resolveTimeoutPromise = resolve;
   });
+
+  var timeoutId = this.__nextReadyTimeoutId;
+  this.__nextReadyTimeoutId++;
+
+  var onReadyTimeout = function() {
+    delete this.__readyTimeouts[timeoutId];
+    resolveTimeoutPromise({
+      success: false,
+      reason: sprintf('onReady timeout expired after %s ms', timeout),
+    });
+  }.bind(this);
+  var readyTimeout = setTimeout(onReadyTimeout, timeout);
+  var onClose = function() {
+    resolveTimeoutPromise({
+      success: false,
+      reason: 'Instance closed',
+    });
+  };
+
+  this.__readyTimeouts[timeoutId] = {
+    readyTimeout: readyTimeout,
+    onClose: onClose,
+  };
+
   return Promise.race([this.__readyPromise, timeoutPromise]);
 };
 
diff --git a/packages/optimizely-sdk/lib/optimizely/index.tests.js b/packages/optimizely-sdk/lib/optimizely/index.tests.js
@@ -4810,7 +4810,7 @@ describe('lib/optimizely', function() {
       it('returns fallback values from API methods that return meaningful values', function() {
         assert.isNull(optlyInstance.activate('my_experiment', 'user1'));
         assert.isNull(optlyInstance.getVariation('my_experiment', 'user1'));
-        assert.isNull(optlyInstance.setForcedVariation('my_experiment', 'user1', 'variation_1'));
+        assert.isFalse(optlyInstance.setForcedVariation('my_experiment', 'user1', 'variation_1'));
         assert.isNull(optlyInstance.getForcedVariation('my_experiment', 'user1'));
         assert.isFalse(optlyInstance.isFeatureEnabled('my_feature', 'user1'));
         assert.deepEqual(optlyInstance.getEnabledFeatures('user1'), []);
@@ -4839,6 +4839,30 @@ describe('lib/optimizely', function() {
         clock.restore();
       });
 
+      it('fulfills the promise with the value from the project config manager ready promise after the project config manager ready promise is fulfilled', function() {
+        projectConfigManager.ProjectConfigManager.callsFake(function(config) {
+          var currentConfig = config.datafile ? projectConfig.createProjectConfig(config.datafile) : null;
+          return {
+            stop: sinon.stub(),
+            getConfig: sinon.stub().returns(currentConfig),
+            onUpdate: sinon.stub().returns(function() {}),
+            onReady: sinon.stub().returns(Promise.resolve({ success: true })),
+          };
+        });
+        optlyInstance = new Optimizely({
+          clientEngine: 'node-sdk',
+          errorHandler: errorHandler,
+          eventDispatcher: eventDispatcher,
+          jsonSchemaValidator: jsonSchemaValidator,
+          logger: createdLogger,
+          sdkKey: '12345',
+          isValidInstance: true,
+        });
+        return optlyInstance.onReady().then(function(result) {
+          assert.deepEqual(result, { success: true });
+        });
+      });
+
       it('fulfills the promise with an unsuccessful result after the timeout has expired when the project config manager onReady promise still has not resolved', function() {
         optlyInstance = new Optimizely({
           clientEngine: 'node-sdk',
@@ -4876,6 +4900,50 @@ describe('lib/optimizely', function() {
           });
         });
       });
+
+      it('fulfills the promise with an unsuccessful result after the instance is closed', function() {
+        optlyInstance = new Optimizely({
+          clientEngine: 'node-sdk',
+          errorHandler: errorHandler,
+          eventDispatcher: eventDispatcher,
+          jsonSchemaValidator: jsonSchemaValidator,
+          logger: createdLogger,
+          sdkKey: '12345',
+          isValidInstance: true,
+        });
+        var readyPromise = optlyInstance.onReady({ timeout: 100 });
+        optlyInstance.close();
+        return readyPromise.then(function(result) {
+          assert.include(result, {
+            success: false,
+          });
+        });
+      });
+
+      it('can be called several times with different timeout values and the returned promises behave correctly', function() {
+        optlyInstance = new Optimizely({
+          clientEngine: 'node-sdk',
+          errorHandler: errorHandler,
+          eventDispatcher: eventDispatcher,
+          jsonSchemaValidator: jsonSchemaValidator,
+          logger: createdLogger,
+          sdkKey: '12345',
+          isValidInstance: true,
+        });
+        var readyPromise1 = optlyInstance.onReady({ timeout: 100 });
+        var readyPromise2 = optlyInstance.onReady({ timeout: 200 });
+        var readyPromise3 = optlyInstance.onReady({ timeout: 300 });
+        clock.tick(101);
+        return readyPromise1.then(function() {
+          clock.tick(100);
+          return readyPromise2;
+        }).then(function() {
+          // readyPromise3 has not resolved yet because only 201 ms have elapsed.
+          // Calling close on the instance should resolve readyPromise3
+          optlyInstance.close();
+          return readyPromise3;
+        });
+      });
     });
 
     describe('project config updates', function() {
