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.

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

Conversation

zashraf1985
Copy link
Contributor

  1. Added try/catch blocks at the entry points in the SDK.
  2. Added ErrorHandler and logging to all the catch blocks.

zashraf1985 and others added 8 commits June 4, 2018 20:54
…oints

# Conflicts:
#	packages/optimizely-sdk/lib/core/event_builder/index.js
2) Added Error Handler and Logger log in catch blocks
3) Now sending Error Handler to notification center in options object to use in catch blocks.
Copy link
Contributor

@spencerwilson-optimizely spencerwilson-optimizely left a comment

Choose a reason for hiding this comment

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

This is looking much more like what we'll eventually want to ship than previous, thanks!

One thing that's still at-risk are the async callbacks of the default EventDispatchers (both in browsers and Node.js) are an asynchronous entry point to our code, when they invoke their callbacks. Those will need to have try/catches, too.

Specifically, I'm referring to

@@ -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

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.

return new Optimizely(config);
return new Optimizely(config);
} catch (e) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please delete empty lines like these? The codebase currently doesn't have this style, and any PR that changes that should probably do so wholesale at the same time across the whole codebase, to maintain consistency.

Copy link
Contributor Author

@zashraf1985 zashraf1985 Jun 28, 2018

Choose a reason for hiding this comment

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

Done. That was a mistake on my part and i have removed them. I did not make any intentional changes to the style, it happened because i used regex based find/replace to remove fault injection code, which removed the fault injection lines but it did not remove the line breaks above and below.

if (clientEngine !== enums.NODE_CLIENT_ENGINE && clientEngine !== enums.JAVASCRIPT_CLIENT_ENGINE) {
config.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.INVALID_CLIENT_ENGINE, MODULE_NAME, clientEngine));
clientEngine = enums.NODE_CLIENT_ENGINE;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here and elsewhere: please delete new blank lines.

Copy link
Contributor Author

@zashraf1985 zashraf1985 Jun 28, 2018

Choose a reason for hiding this comment

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

Done. That was a mistake on my part and i have removed them. I did not make any intentional changes to the style, it happened because i used regex based find/replace to remove fault injection code, which removed the fault injection lines but it did not remove the line breaks above and below.

this.errorHandler.handleError(ex);
}
} catch (e) {
if (e.message.startsWith("PROJECT_CONFIG")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this branch about? Could you add a comment above it explaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some methods throw expected exceptions just like checked exceptions in java. We do not want to catch and eat those exceptions because we are expecting consumer app to expect and handle them, so we simply re-throw them if they are expected ones. This is at least my understanding. Please let me know if you see any problem with my observation. And yes, i will add a comment in the code once you approve the approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

we are expecting consumer app to expect and handle them

On the contrary, I don't think we want to throw any exceptions from the SDK. So this if block can just be deleted, and L327-L330 can remain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the if block.

return null;
}
} catch (e) {
if (e.message.startsWith(MODULE_NAME)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise here, what is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as the last one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise here, please remove this if branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@@ -19,7 +19,8 @@
"lint": "eslint lib/**",
"cover": "istanbul cover _mocha ./lib/*.tests.js ./lib/**/*.tests.js ./lib/**/**/*tests.js",
"coveralls": "npm run cover -- --report lcovonly && cat ./coverage/lcov.info | coveralls",
"prepare": "npm test && npm run build"
"prepare": "npm test && npm run build",
"fault-injection": "node lib/fault_injection/test_processes_manager.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to this file and to package-lock.json can be discarded, since I don't think (please correct if I'm wrong) you intend to merge lib/fault_injection_test_processes_manager.js. If you do intend to commit that to the repo, it would probably go in a directory other than lib/, which contains the actual source code of the SDK. Maybe a new directory calls 'tools' or something.

git checkout master -- packages/optimizely-sdk/package.json packages/optimizely-sdk/package-lock.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i agree. This is a mistake on my part. The plan is to maintain a parallel branch with fault injection ecosystem in it and remove it completely when creating a pull request. This should have been removed while creating P.R and i missed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the changes to both package.json and package-lock.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return new Optimizely(config);
} catch (e) {

config.logger.log(LOG_LEVEL.ERROR, e.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

enums is in scope already, so you could use enums.LOG_EVEL.ERROR (and delete the new import on L25) to be consistent with the other references to log levels in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -25,6 +25,8 @@ var sprintf = require('sprintf');

var Optimizely = require('./optimizely');

var LOG_LEVEL = enums.LOG_LEVEL;
Copy link
Contributor

Choose a reason for hiding this comment

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

The Mobile SDKs return a "fake" client object whose methods do nothing except log warnings. It's not clear yet that we want to do the same thing for the Full Stack SDKs, so how about we revert the changes in this file for now and decide on what we want to do with createInstance later.

Copy link
Contributor Author

@zashraf1985 zashraf1985 Jun 28, 2018

Choose a reason for hiding this comment

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

I have removed it on your suggestion but what if something crashes within the SDK while initializing. It will end up crashing consumer app if we remove try catches from here. what do you think about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

While that is true, I think we can accept that risk for now, and make a note to ourselves to return to this problem later once we know how we wish to handle this risk.

this.errorHandler.handleError(ex);
}
} catch (e) {
if (e.message.startsWith("PROJECT_CONFIG")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we are expecting consumer app to expect and handle them

On the contrary, I don't think we want to throw any exceptions from the SDK. So this if block can just be deleted, and L327-L330 can remain.

return null;
}
} catch (e) {
if (e.message.startsWith(MODULE_NAME)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise here, please remove this if branch.

@@ -19,7 +19,8 @@
"lint": "eslint lib/**",
"cover": "istanbul cover _mocha ./lib/*.tests.js ./lib/**/*.tests.js ./lib/**/**/*tests.js",
"coveralls": "npm run cover -- --report lcovonly && cat ./coverage/lcov.info | coveralls",
"prepare": "npm test && npm run build"
"prepare": "npm test && npm run build",
"fault-injection": "node lib/fault_injection/test_processes_manager.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the changes to both package.json and package-lock.json.

zashraf1985 and others added 4 commits June 28, 2018 16:29
…on-optimizely

1) Removed extra line breaks
2) Reverted package.json to the state it was in before faultinjection.
3) Reverted package-lock.json to the state it was in before faultinjection.
4) removed an additional LOG_LEVEL instance in `index.browser.js`. Using `enums.LOG_LEVEL` instead.
5) Removed try/catch block from `index.node.js` and reverted it back the state it was in before fault injection.
6) Removed extra `if` blocks which were re-throwing exceptions inside catch blocks.
@spencerwilson-optimizely spencerwilson-optimizely merged commit d1af730 into master Jun 28, 2018
@spencerwilson-optimizely spencerwilson-optimizely deleted the zeeshan/faultInjectionAndTreatmentAtEntryPoints branch June 28, 2018 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants