-
Notifications
You must be signed in to change notification settings - Fork 83
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
Fault Treatment at entry points of SDK #127
Conversation
zashraf1985
commented
Jun 26, 2018
- Added try/catch blocks at the entry points in the SDK.
- Added ErrorHandler and logging to all the catch blocks.
…edly for each exception spot
…oes not have an impact on previous results.
…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.
There was a problem hiding this 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
-
javascript-sdk/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js
Lines 55 to 57 in 6d6bb24
if (response && response.statusCode && response.statusCode >= 200 && response.statusCode < 400) { callback(response); }
@@ -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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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")) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
packages/optimizely-sdk/package.json
Outdated
@@ -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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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")) { |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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.
packages/optimizely-sdk/package.json
Outdated
@@ -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" |
There was a problem hiding this comment.
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.
…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.