Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit de8546c

Browse files
authored
fix (onReady): Clear ready timeout when instance closed, and other small fixes (optimizely#25)
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
1 parent c4531e6 commit de8546c

File tree

3 files changed

+137
-20
lines changed

3 files changed

+137
-20
lines changed

packages/optimizely-sdk/lib/core/project_config/project_config_manager.js

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,8 +285,23 @@ ProjectConfigManager.prototype.getConfig = function() {
285285
};
286286

287287
/**
288-
* Returns a Promise that resolves when this ProjectConfigManager has a non-null
289-
* project config object available for the first time.
288+
* Returns a Promise that fulfills when this ProjectConfigManager is ready to
289+
* use (meaning it has a valid project config object), or has failed to become
290+
* ready.
291+
*
292+
* Failure can be caused by the following:
293+
* - At least one of sdkKey or datafile is not provided in the constructor argument
294+
* - The provided datafile was invalid
295+
* - The datafile provided by the datafile manager was invalid
296+
* - The datafile manager failed to fetch a datafile
297+
*
298+
* The returned Promise is fulfilled with a result object containing these
299+
* properties:
300+
* - success (boolean): True if this instance is ready to use with a valid
301+
* project config object, or false if it failed to
302+
* become ready
303+
* - reason (string=): If success is false, this is a string property with
304+
* an explanatory message.
290305
* @return {Promise}
291306
*/
292307
ProjectConfigManager.prototype.onReady = function() {

packages/optimizely-sdk/lib/optimizely/index.js

Lines changed: 51 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,9 @@ function Optimizely(config) {
113113
maxQueueSize: config.eventBatchSize || DEFAULT_EVENT_MAX_QUEUE_SIZE,
114114
});
115115
this.eventProcessor.start();
116+
117+
this.__readyTimeouts = {};
118+
this.__nextReadyTimeoutId = 0;
116119
}
117120

118121
/**
@@ -420,7 +423,7 @@ Optimizely.prototype.setForcedVariation = function(experimentKey, userId, variat
420423

421424
var configObj = this.projectConfigManager.getConfig();
422425
if (!configObj) {
423-
return null;
426+
return false;
424427
}
425428

426429
try {
@@ -582,7 +585,7 @@ Optimizely.prototype.isFeatureEnabled = function(featureKey, userId, attributes)
582585
source: decision.decisionSource,
583586
sourceInfo: sourceInfo
584587
};
585-
588+
586589
this.notificationCenter.sendNotifications(
587590
NOTIFICATION_TYPES.DECISION,
588591
{
@@ -857,6 +860,12 @@ Optimizely.prototype.close = function() {
857860
if (this.projectConfigManager) {
858861
this.projectConfigManager.stop();
859862
}
863+
Object.keys(this.__readyTimeouts).forEach(function(readyTimeoutId) {
864+
var readyTimeoutRecord = this.__readyTimeouts[readyTimeoutId];
865+
clearTimeout(readyTimeoutRecord.readyTimeout);
866+
readyTimeoutRecord.onClose();
867+
}.bind(this));
868+
this.__readyTimeouts = {};
860869
} catch (e) {
861870
this.logger.log(LOG_LEVEL.ERROR, e.message);
862871
this.errorHandler.handleError(e);
@@ -866,22 +875,26 @@ Optimizely.prototype.close = function() {
866875
/**
867876
* Returns a Promise that fulfills when this instance is ready to use (meaning
868877
* it has a valid datafile), or has failed to become ready within a period of
869-
* time (configurable by the timeout property of the options argument). If a
870-
* valid datafile was provided in the constructor, the instance is immediately
871-
* ready. If an sdkKey was provided, a manager will be used to fetch a datafile,
872-
* and the returned promise will resolve if that fetch succeeds or fails before
873-
* the timeout. The default timeout is 30 seconds, which will be used if no
874-
* timeout is provided in the argument options object.
878+
* time (configurable by the timeout property of the options argument), or when
879+
* this instance is closed via the close method.
880+
*
881+
* If a valid datafile was provided in the constructor, the returned Promise is
882+
* immediately fulfilled. If an sdkKey was provided, a manager will be used to
883+
* fetch a datafile, and the returned promise will fulfill if that fetch
884+
* succeeds or fails before the timeout. The default timeout is 30 seconds,
885+
* which will be used if no timeout is provided in the argument options object.
886+
*
875887
* The returned Promise is fulfilled with a result object containing these
876888
* properties:
877889
* - success (boolean): True if this instance is ready to use with a valid
878890
* datafile, or false if this instance failed to become
879-
* ready.
891+
* ready or was closed prior to becoming ready.
880892
* - reason (string=): If success is false, this is a string property with
881893
* an explanatory message. Failure could be due to
882894
* expiration of the timeout, network errors,
883-
* unsuccessful responses, datafile parse errors, or
884-
* datafile validation errors.
895+
* unsuccessful responses, datafile parse errors,
896+
* datafile validation errors, or the instance being
897+
* closed
885898
* @param {Object=} options
886899
* @param {number|undefined} options.timeout
887900
* @return {Promise}
@@ -894,14 +907,35 @@ Optimizely.prototype.onReady = function(options) {
894907
if (!fns.isFinite(timeout)) {
895908
timeout = DEFAULT_ONREADY_TIMEOUT;
896909
}
910+
911+
var resolveTimeoutPromise;
897912
var timeoutPromise = new Promise(function(resolve) {
898-
setTimeout(function() {
899-
resolve({
900-
success: false,
901-
reason: sprintf('onReady timeout expired after %s ms', timeout),
902-
});
903-
}, timeout);
913+
resolveTimeoutPromise = resolve;
904914
});
915+
916+
var timeoutId = this.__nextReadyTimeoutId;
917+
this.__nextReadyTimeoutId++;
918+
919+
var onReadyTimeout = function() {
920+
delete this.__readyTimeouts[timeoutId];
921+
resolveTimeoutPromise({
922+
success: false,
923+
reason: sprintf('onReady timeout expired after %s ms', timeout),
924+
});
925+
}.bind(this);
926+
var readyTimeout = setTimeout(onReadyTimeout, timeout);
927+
var onClose = function() {
928+
resolveTimeoutPromise({
929+
success: false,
930+
reason: 'Instance closed',
931+
});
932+
};
933+
934+
this.__readyTimeouts[timeoutId] = {
935+
readyTimeout: readyTimeout,
936+
onClose: onClose,
937+
};
938+
905939
return Promise.race([this.__readyPromise, timeoutPromise]);
906940
};
907941

packages/optimizely-sdk/lib/optimizely/index.tests.js

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4810,7 +4810,7 @@ describe('lib/optimizely', function() {
48104810
it('returns fallback values from API methods that return meaningful values', function() {
48114811
assert.isNull(optlyInstance.activate('my_experiment', 'user1'));
48124812
assert.isNull(optlyInstance.getVariation('my_experiment', 'user1'));
4813-
assert.isNull(optlyInstance.setForcedVariation('my_experiment', 'user1', 'variation_1'));
4813+
assert.isFalse(optlyInstance.setForcedVariation('my_experiment', 'user1', 'variation_1'));
48144814
assert.isNull(optlyInstance.getForcedVariation('my_experiment', 'user1'));
48154815
assert.isFalse(optlyInstance.isFeatureEnabled('my_feature', 'user1'));
48164816
assert.deepEqual(optlyInstance.getEnabledFeatures('user1'), []);
@@ -4839,6 +4839,30 @@ describe('lib/optimizely', function() {
48394839
clock.restore();
48404840
});
48414841

4842+
it('fulfills the promise with the value from the project config manager ready promise after the project config manager ready promise is fulfilled', function() {
4843+
projectConfigManager.ProjectConfigManager.callsFake(function(config) {
4844+
var currentConfig = config.datafile ? projectConfig.createProjectConfig(config.datafile) : null;
4845+
return {
4846+
stop: sinon.stub(),
4847+
getConfig: sinon.stub().returns(currentConfig),
4848+
onUpdate: sinon.stub().returns(function() {}),
4849+
onReady: sinon.stub().returns(Promise.resolve({ success: true })),
4850+
};
4851+
});
4852+
optlyInstance = new Optimizely({
4853+
clientEngine: 'node-sdk',
4854+
errorHandler: errorHandler,
4855+
eventDispatcher: eventDispatcher,
4856+
jsonSchemaValidator: jsonSchemaValidator,
4857+
logger: createdLogger,
4858+
sdkKey: '12345',
4859+
isValidInstance: true,
4860+
});
4861+
return optlyInstance.onReady().then(function(result) {
4862+
assert.deepEqual(result, { success: true });
4863+
});
4864+
});
4865+
48424866
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() {
48434867
optlyInstance = new Optimizely({
48444868
clientEngine: 'node-sdk',
@@ -4876,6 +4900,50 @@ describe('lib/optimizely', function() {
48764900
});
48774901
});
48784902
});
4903+
4904+
it('fulfills the promise with an unsuccessful result after the instance is closed', function() {
4905+
optlyInstance = new Optimizely({
4906+
clientEngine: 'node-sdk',
4907+
errorHandler: errorHandler,
4908+
eventDispatcher: eventDispatcher,
4909+
jsonSchemaValidator: jsonSchemaValidator,
4910+
logger: createdLogger,
4911+
sdkKey: '12345',
4912+
isValidInstance: true,
4913+
});
4914+
var readyPromise = optlyInstance.onReady({ timeout: 100 });
4915+
optlyInstance.close();
4916+
return readyPromise.then(function(result) {
4917+
assert.include(result, {
4918+
success: false,
4919+
});
4920+
});
4921+
});
4922+
4923+
it('can be called several times with different timeout values and the returned promises behave correctly', function() {
4924+
optlyInstance = new Optimizely({
4925+
clientEngine: 'node-sdk',
4926+
errorHandler: errorHandler,
4927+
eventDispatcher: eventDispatcher,
4928+
jsonSchemaValidator: jsonSchemaValidator,
4929+
logger: createdLogger,
4930+
sdkKey: '12345',
4931+
isValidInstance: true,
4932+
});
4933+
var readyPromise1 = optlyInstance.onReady({ timeout: 100 });
4934+
var readyPromise2 = optlyInstance.onReady({ timeout: 200 });
4935+
var readyPromise3 = optlyInstance.onReady({ timeout: 300 });
4936+
clock.tick(101);
4937+
return readyPromise1.then(function() {
4938+
clock.tick(100);
4939+
return readyPromise2;
4940+
}).then(function() {
4941+
// readyPromise3 has not resolved yet because only 201 ms have elapsed.
4942+
// Calling close on the instance should resolve readyPromise3
4943+
optlyInstance.close();
4944+
return readyPromise3;
4945+
});
4946+
});
48794947
});
48804948

48814949
describe('project config updates', function() {

0 commit comments

Comments
 (0)