Skip to content

Fault Treatment at entry points of SDK #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jun 28, 2018
Merged

Fault Treatment at entry points of SDK #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
127c9f3
Developed a small framework to run test suite programmatically repeat…
zashraf1985 Jun 3, 2018
cf161b2
Added faults to some places
zashraf1985 Jun 4, 2018
674d4a6
Added exceptions all over the SDK
zashraf1985 Jun 5, 2018
677773f
corrected a mistake in the code which throws error. fortunately, it d…
zashraf1985 Jun 5, 2018
efd272e
Merge branch 'master' into zeeshan/faultInjectionAndTreatmentAtEntryP…
zashraf1985 Jun 22, 2018
33fa72f
1) Added try/catch blocks at entry level
zashraf1985 Jun 25, 2018
315ea6d
Removed fault injection code to create Pull Request
zashraf1985 Jun 25, 2018
714f058
Merge branch 'master' into zeeshan/faultInjectionAndTreatmentAtEntryP…
zashraf1985 Jun 26, 2018
3bb16b3
Remove whitespace changes
spencerwilson-optimizely Jun 26, 2018
01384bd
Made following modifications as per changes requested by @spencerwils…
zashraf1985 Jun 28, 2018
909412a
Delete remaining whitespace changes
spencerwilson-optimizely Jun 28, 2018
1204eec
Delete commented out things
spencerwilson-optimizely Jun 28, 2018
c51da52
Make EventDispatcher comments more descriptive TODO comments
spencerwilson-optimizely Jun 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 82 additions & 55 deletions packages/optimizely-sdk/lib/core/notification_center/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ var MODULE_NAME = 'NOTIFICATION_CENTER';
* @constructor
* @param {Object} options
* @param {Object} options.logger An instance of a logger to log messages with
* @param {object} options.errorHandler An instance of errorHandler to handle any unexpected error
* @returns {Object}
*/
function NotificationCenter(options) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotificationCenter is actually not an entry point to the SDK. This line within the Optimizely constructor function, which in turn is only called from the package's public createInstance function, is the sole place that calls into any code from this module. This means that all of the changes in this file can be reverted:

git checkout master -- packages/optimizely-sdk/lib/core/notification_center/index.js

Copy link
Contributor Author

@zashraf1985 zashraf1985 Jun 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree to the extent of NotificationCenter constructor and that is the reason why you wont see a try/catch block in NotificationCenter constructor. The reason to put try/catch blocks in other methods is because an instance of NotificationCenter is available to the consumer as a public member of OptimizelyInstance. Consumer application can call addNotificationListener, removeNotificationListener, clearAllNotificationListeners, clearNotificationListeners, sendNotifications using NotificationCenter instance. The idea was to have try/catch blocks on all the functions which can be directly called by consumer application so that if any of these function crashes because of an unexpected exception/error, it does not in turn crash the consumer application. If you still think we should revert it, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, you're right, excellent catch. NotificationCenter instance methods are entry points which I overlooked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you agree this is correct and does not need to be reverted back. I am keeping it as is. let me know if my understanding is wrong.

this.logger = options.logger;
this.errorHandler = options.errorHandler;
this.__notificationListeners = {};
fns.forOwn(enums.NOTIFICATION_TYPES, function(notificationTypeEnum) {
this.__notificationListeners[notificationTypeEnum] = [];
Expand All @@ -51,36 +53,42 @@ function NotificationCenter(options) {
* can happen if the first argument is not a valid notification type, or if the same callback
* function was already added as a listener by a prior call to this function.
*/
NotificationCenter.prototype.addNotificationListener = function(notificationType, callback) {
var isNotificationTypeValid = fns.values(enums.NOTIFICATION_TYPES)
.indexOf(notificationType) > -1;
if (!isNotificationTypeValid) {
return -1;
}
NotificationCenter.prototype.addNotificationListener = function (notificationType, callback) {
try {
var isNotificationTypeValid = fns.values(enums.NOTIFICATION_TYPES)
.indexOf(notificationType) > -1;
if (!isNotificationTypeValid) {
return -1;
}

if (!this.__notificationListeners[notificationType]) {
this.__notificationListeners[notificationType] = [];
}
if (!this.__notificationListeners[notificationType]) {
this.__notificationListeners[notificationType] = [];
}

var callbackAlreadyAdded = false;
fns.forEach(this.__notificationListeners[notificationType], function(listenerEntry) {
if (listenerEntry.callback === callback) {
callbackAlreadyAdded = true;
return false;
var callbackAlreadyAdded = false;
fns.forEach(this.__notificationListeners[notificationType], function (listenerEntry) {
if (listenerEntry.callback === callback) {
callbackAlreadyAdded = true;
return false;
}
});
if (callbackAlreadyAdded) {
return -1;
}
});
if (callbackAlreadyAdded) {
return -1;
}

this.__notificationListeners[notificationType].push({
id: this.__listenerId,
callback: callback,
});
this.__notificationListeners[notificationType].push({
id: this.__listenerId,
callback: callback,
});

var returnId = this.__listenerId;
this.__listenerId += 1;
return returnId;
var returnId = this.__listenerId;
this.__listenerId += 1;
return returnId;
} catch (e) {
this.logger.log(LOG_LEVEL.ERROR, e.message);
this.errorHandler.handleError(e);
return -1;
}
};

/**
Expand All @@ -89,45 +97,59 @@ NotificationCenter.prototype.addNotificationListener = function(notificationType
* @returns {boolean} Returns true if the listener was found and removed, and false
* otherwise.
*/
NotificationCenter.prototype.removeNotificationListener = function(listenerId) {
var indexToRemove;
var typeToRemove;
fns.forOwn(this.__notificationListeners, function(listenersForType, notificationType) {
fns.forEach(listenersForType, function(listenerEntry, i) {
if (listenerEntry.id === listenerId) {
indexToRemove = i;
typeToRemove = notificationType;
NotificationCenter.prototype.removeNotificationListener = function (listenerId) {
try {
var indexToRemove;
var typeToRemove;
fns.forOwn(this.__notificationListeners, function (listenersForType, notificationType) {
fns.forEach(listenersForType, function (listenerEntry, i) {
if (listenerEntry.id === listenerId) {
indexToRemove = i;
typeToRemove = notificationType;
return false;
}
});
if (indexToRemove !== undefined && typeToRemove !== undefined) {
return false;
}
});

if (indexToRemove !== undefined && typeToRemove !== undefined) {
return false;
this.__notificationListeners[typeToRemove].splice(indexToRemove, 1);
return true;
}
});

if (indexToRemove !== undefined && typeToRemove !== undefined) {
this.__notificationListeners[typeToRemove].splice(indexToRemove, 1);
return true;
} catch (e) {
this.logger.log(LOG_LEVEL.ERROR, e.message);
this.errorHandler.handleError(e);
}

return false;
};

/**
* Removes all previously added notification listeners, for all notification types
*/
NotificationCenter.prototype.clearAllNotificationListeners = function() {
fns.forOwn(enums.NOTIFICATION_TYPES, function(notificationTypeEnum) {
this.__notificationListeners[notificationTypeEnum] = [];
}.bind(this));
NotificationCenter.prototype.clearAllNotificationListeners = function () {
try{
fns.forOwn(enums.NOTIFICATION_TYPES, function (notificationTypeEnum) {
this.__notificationListeners[notificationTypeEnum] = [];
}.bind(this));
} catch (e) {
this.logger.log(LOG_LEVEL.ERROR, e.message);
this.errorHandler.handleError(e);
}
};

/**
* Remove all previously added notification listeners for the argument type
* @param {string} notificationType One of enums.NOTIFICATION_TYPES
*/
NotificationCenter.prototype.clearNotificationListeners = function(notificationType) {
this.__notificationListeners[notificationType] = [];
NotificationCenter.prototype.clearNotificationListeners = function (notificationType) {
try {
this.__notificationListeners[notificationType] = [];
} catch (e) {
this.logger.log(LOG_LEVEL.ERROR, e.message);
this.errorHandler.handleError(e);
}
};

/**
Expand All @@ -136,15 +158,20 @@ NotificationCenter.prototype.clearNotificationListeners = function(notificationT
* @param {string} notificationType One of enums.NOTIFICATION_TYPES
* @param {Object} notificationData Will be passed to callbacks called
*/
NotificationCenter.prototype.sendNotifications = function(notificationType, notificationData) {
fns.forEach(this.__notificationListeners[notificationType], function(listenerEntry) {
var callback = listenerEntry.callback;
try {
callback(notificationData);
} catch (ex) {
this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.NOTIFICATION_LISTENER_EXCEPTION, MODULE_NAME, notificationType, ex.message));
}
}.bind(this));
NotificationCenter.prototype.sendNotifications = function (notificationType, notificationData) {
try {
fns.forEach(this.__notificationListeners[notificationType], function (listenerEntry) {
var callback = listenerEntry.callback;
try {
callback(notificationData);
} catch (ex) {
this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.NOTIFICATION_LISTENER_EXCEPTION, MODULE_NAME, notificationType, ex.message));
}
}.bind(this));
} catch (e) {
this.logger.log(LOG_LEVEL.ERROR, e.message);
this.errorHandler.handleError(e);
}
};

module.exports = {
Expand Down
58 changes: 32 additions & 26 deletions packages/optimizely-sdk/lib/index.browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,36 +39,42 @@ module.exports = {
* @return {Object} the Optimizely object
*/
createInstance: function(config) {
var logLevel = 'logLevel' in config ? config.logLevel : enums.LOG_LEVEL.INFO;
var defaultLogger = logger.createLogger({ logLevel: enums.LOG_LEVEL.INFO });
if (config) {
try {
configValidator.validate(config);
config.isValidInstance = true;
} catch (ex) {
var errorMessage = MODULE_NAME + ':' + ex.message;
if (config.logger) {
config.logger.log(enums.LOG_LEVEL.ERROR, errorMessage);
} else {
defaultLogger.log(enums.LOG_LEVEL.ERROR, errorMessage);
try {
var logLevel = 'logLevel' in config ? config.logLevel : enums.LOG_LEVEL.INFO;
var defaultLogger = logger.createLogger({logLevel: enums.LOG_LEVEL.INFO});
if (config) {
try {
configValidator.validate(config);
config.isValidInstance = true;
} catch (ex) {
var errorMessage = MODULE_NAME + ':' + ex.message;
if (config.logger) {
config.logger.log(enums.LOG_LEVEL.ERROR, errorMessage);
} else {
defaultLogger.log(enums.LOG_LEVEL.ERROR, errorMessage);
}
config.isValidInstance = false;
}
config.isValidInstance = false;
}
}

// Explicitly check for null or undefined
if (config.skipJSONValidation == null) { // eslint-disable-line eqeqeq
config.skipJSONValidation = true;
}
// Explicitly check for null or undefined
if (config.skipJSONValidation == null) { // eslint-disable-line eqeqeq
config.skipJSONValidation = true;
}

config = fns.assignIn({
clientEngine: enums.JAVASCRIPT_CLIENT_ENGINE,
clientVersion: enums.CLIENT_VERSION,
errorHandler: defaultErrorHandler,
eventDispatcher: defaultEventDispatcher,
logger: logger.createLogger({ logLevel: logLevel })
}, config);
config = fns.assignIn({
clientEngine: enums.JAVASCRIPT_CLIENT_ENGINE,
clientVersion: enums.CLIENT_VERSION,
errorHandler: defaultErrorHandler,
eventDispatcher: defaultEventDispatcher,
logger: logger.createLogger({logLevel: logLevel})
}, config);

return new Optimizely(config);
return new Optimizely(config);
} catch (e) {
config.logger.log(enums.LOG_LEVEL.ERROR, e.message);
config.errorHandler.handleError(e);
return null;
}
}
};
Loading