Fault Treatment at entry points of SDK #127

zashraf1985 · 2018-06-26T07:20:58Z

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.

…oints

spencerwilson-optimizely

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); 
        
           }

https://github.com/optimizely/javascript-sdk/blob/master/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js#L52-L54

spencerwilson-optimizely · 2018-06-26T23:30:23Z

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

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


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

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.

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

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.

spencerwilson-optimizely · 2018-06-26T23:41:36Z

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

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


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.

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.

spencerwilson-optimizely · 2018-06-26T23:42:06Z

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

-  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;
-  }



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

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.

spencerwilson-optimizely · 2018-06-26T23:42:33Z

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

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


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

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.

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.

Removed the if block.

spencerwilson-optimizely · 2018-06-26T23:42:53Z

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

+      return null;
+    }
+  } catch (e) {
+    if (e.message.startsWith(MODULE_NAME)) {


Likewise here, what is this?

Same reason as the last one.

Likewise here, please remove this if branch.

spencerwilson-optimizely · 2018-06-26T23:47:57Z

packages/optimizely-sdk/package.json

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


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

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.

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

spencerwilson-optimizely · 2018-06-27T23:18:41Z

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

+      return new Optimizely(config);
+    } catch (e) {
+
+      config.logger.log(LOG_LEVEL.ERROR, e.message);


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.

spencerwilson-optimizely · 2018-06-27T23:57:03Z

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

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

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

+var LOG_LEVEL = enums.LOG_LEVEL;


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.

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?

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.

spencerwilson-optimizely · 2018-06-28T00:13:14Z

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

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


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.

spencerwilson-optimizely · 2018-06-28T00:13:32Z

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

+      return null;
+    }
+  } catch (e) {
+    if (e.message.startsWith(MODULE_NAME)) {


Likewise here, please remove this if branch.

spencerwilson-optimizely · 2018-06-28T00:14:14Z

packages/optimizely-sdk/package.json

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


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

…on-optimizely

…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.

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

Developed a small framework to run test suite programmatically repeat…

127c9f3

…edly for each exception spot

Added faults to some places

cf161b2

Added exceptions all over the SDK

674d4a6

corrected a mistake in the code which throws error. fortunately, it d…

677773f

…oes not have an impact on previous results.

Merge branch 'master' into zeeshan/faultInjectionAndTreatmentAtEntryP…

efd272e

…oints # Conflicts: # packages/optimizely-sdk/lib/core/event_builder/index.js

1) Added try/catch blocks at entry level

33fa72f

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.

Removed fault injection code to create Pull Request

315ea6d

Merge branch 'master' into zeeshan/faultInjectionAndTreatmentAtEntryP…

714f058

…oints

kellyroach-optimizely requested a review from spencerwilson-optimizely June 26, 2018 17:35

Remove whitespace changes

3bb16b3

spencerwilson-optimizely suggested changes Jun 26, 2018

View reviewed changes

spencerwilson-optimizely suggested changes Jun 28, 2018

View reviewed changes

zashraf1985 and others added 4 commits June 28, 2018 16:29

Delete remaining whitespace changes

909412a

Delete commented out things

1204eec

Make EventDispatcher comments more descriptive TODO comments

c51da52

spencerwilson-optimizely approved these changes Jun 28, 2018

View reviewed changes

spencerwilson-optimizely merged commit d1af730 into master Jun 28, 2018

spencerwilson-optimizely deleted the zeeshan/faultInjectionAndTreatmentAtEntryPoints branch June 28, 2018 17:16

	if (response && response.statusCode && response.statusCode >= 200 && response.statusCode < 400) {
	callback(response);
	}

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

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

		var LOG_LEVEL = enums.LOG_LEVEL;

Fault Treatment at entry points of SDK #127

Fault Treatment at entry points of SDK #127

Uh oh!

Conversation

zashraf1985 commented Jun 26, 2018

Uh oh!

spencerwilson-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zashraf1985 Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zashraf1985 Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zashraf1985 Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zashraf1985 Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zashraf1985 Jun 27, 2018 •

edited

Loading

zashraf1985 Jun 28, 2018 •

edited

Loading

zashraf1985 Jun 28, 2018 •

edited

Loading

zashraf1985 Jun 28, 2018 •

edited

Loading