Repository: aws/aws-xray-sdk-node
Pull request #126 (2019-04-26 21:02:08)

bugfix: adds workaround for missing cls namespace when cls-hooked imported
*Issue #, if available:*
#111 

*Description of changes:*
This change works around an issue that occurs when `cls-hooked` is directly or indirectly imported after the `aws-xray-sdk`. `cls-hooked` changed how they instantiate their storage to no longer check if the storage already exists or not: [link](https://github.com/Jeff-Lewis/cls-hooked/blob/066c6c4027a7924b06997cc6b175b1841342abdc/context.js#L453)

`continuation-local-storage` and `cls-hooked` use the same global location for storage (process.namespaces), and `cls-hooked` overwrites it the first time it is imported. Since X-Ray SDK creates a namespace the 1st time it is imported and assumes it exists after that, this caused the issue seen in #111 .

This change updated `ContextUtils.getNamespace` to create the namespace if it doesn't already exist. I added `cls-hooked` to the integration tests as well to ensure the workaround.

Note: The `experimental` tag of the SDK is not affected since it uses `cls-hooked` internally.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/packages/core/lib/context_utils.js b/packages/core/lib/context_utils.js
index 91b6c07..a9d2a58 100644
--- a/packages/core/lib/context_utils.js
+++ b/packages/core/lib/context_utils.js
@@ -58,7 +58,7 @@ var contextUtils = {
   },
 
   getNamespace: function getNamespace() {
-    return cls.getNamespace(NAMESPACE);
+    return cls.getNamespace(NAMESPACE) || cls.createNamespace(NAMESPACE);
   },
 
   /**


Test patch:
diff --git a/packages/core/test/unit/context_utils.test.js b/packages/core/test/unit/context_utils.test.js
index 42a2f60..70b3b62 100644
--- a/packages/core/test/unit/context_utils.test.js
+++ b/packages/core/test/unit/context_utils.test.js
@@ -172,4 +172,15 @@ describe('ContextUtils', function() {
       assert.throws(function() { ContextUtils.setContextMissingStrategy({}); } );
     });
   });
+
+  describe('#getNamespace', function() {
+    it('should create the namespace if it is missing', function() {
+      // check that namespace exists originally
+      assert.equal(ContextUtils.getNamespace().name, 'AWSXRay');
+      // delete the namespace
+      delete process.namespaces['AWSXRay'];
+      // namespace should be created
+      assert.equal(ContextUtils.getNamespace().name, 'AWSXRay');
+    });
+  });
 });
diff --git a/packages/test_express/package.json b/packages/test_express/package.json
index 4a5a1d9..f0043cf 100644
--- a/packages/test_express/package.json
+++ b/packages/test_express/package.json
@@ -18,7 +18,9 @@
     "aws-xray-sdk-core": "^2.3.1",
     "aws-xray-sdk-express": "^2.3.1",
     "chai": "^3.5.0",
+    "cls-hooked": "^4.2.2",
     "eslint": "^3.10.2",
+    "express": "^4.16.4",
     "mocha": "^3.0.2"
   },
   "scripts": {
@@ -34,8 +36,5 @@
     "x ray"
   ],
   "license": "Apache-2.0",
-  "repository": "https://github.com/aws/aws-xray-sdk-node/tree/master/packages/express",
-  "dependencies": {
-    "express": "^4.16.4"
-  }
+  "repository": "https://github.com/aws/aws-xray-sdk-node/tree/master/packages/express"
 }
diff --git a/packages/test_express/test/helpers.js b/packages/test_express/test/helpers.js
index 167e00b..7419616 100644
--- a/packages/test_express/test/helpers.js
+++ b/packages/test_express/test/helpers.js
@@ -2,6 +2,7 @@ var dgram = require('dgram');
 var xray = require('aws-xray-sdk-core');
 var xrayExpress = require('aws-xray-sdk-express');
 var express = require('express');
+require('cls-hooked'); // validates compat with continuation-local-storage
 var http = require('http');
 var assert = require('chai').assert;
 
@@ -13,61 +14,61 @@ var assert = require('chai').assert;
  * @param {string} options.route
  */
 function createAppWithRoute(options) {
-  var app = express();
-  if (options.dynamicPattern) {
-    xray.middleware.enableDynamicNaming(options.dynamicPattern);
-  }
-  app.use(xrayExpress.openSegment(options.name));
-  app.use(options.route, options.handler);
-  app.use(xrayExpress.closeSegment());
-  return app;
+    var app = express();
+    if (options.dynamicPattern) {
+        xray.middleware.enableDynamicNaming(options.dynamicPattern);
+    }
+    app.use(xrayExpress.openSegment(options.name));
+    app.use(options.route, options.handler);
+    app.use(xrayExpress.closeSegment());
+    return app;
 }
 
 function createDaemon() {
-  return dgram.createSocket('udp4');
+    return dgram.createSocket('udp4');
 }
 
 function messageCounter(expectedCount, callback) {
-  var messages = [];
-  return (message) => {
-    messages.push(message);
-    if (messages.length == expectedCount) {
-      return callback(messages);
-    } 
-  }
+    var messages = [];
+    return (message) => {
+        messages.push(message);
+        if (messages.length == expectedCount) {
+            return callback(messages);
+        }
+    }
 }
 
 function jitter() {
-  return 100 * Math.random();
+    return 100 * Math.random();
 }
 
 function parseMessage(message) {
-  message = message.toString();
-  var parts = message.split('\n');
-  assert.equal(parts.length, 2, 'Daemon message should contain a protocol and a segment document.');
-  // parse the Segment document
-  return JSON.parse(parts[1]);
+    message = message.toString();
+    var parts = message.split('\n');
+    assert.equal(parts.length, 2, 'Daemon message should contain a protocol and a segment document.');
+    // parse the Segment document
+    return JSON.parse(parts[1]);
 }
 
 /**
  * @param {number} ms 
  */
 function sleep(ms) {
-  return new Promise((resolve) => setTimeout(resolve, ms));
+    return new Promise((resolve) => setTimeout(resolve, ms));
 }
 
 function sleepDedupe() {
-  var inflightSleep = null;
-  return () => {
-    if (!inflightSleep) {
-      inflightSleep = sleep(200);
-      inflightSleep.then(() => {
-        inflightSleep = null
-      });
-    }
+    var inflightSleep = null;
+    return () => {
+        if (!inflightSleep) {
+            inflightSleep = sleep(200);
+            inflightSleep.then(() => {
+                inflightSleep = null
+            });
+        }
 
-    return inflightSleep;
-  }
+        return inflightSleep;
+    }
 }
 
 /**
@@ -75,23 +76,23 @@ function sleepDedupe() {
  * @param {number} waitFor Amount of time to wait before making request in ms
  */
 function triggerEndpoint(url, waitFor) {
-  return new Promise((resolve, reject) => {
-    setTimeout(() => {
-      http.get(url, (response) => {
-        var chunks = [];
-        response.on('data', (chunk) => {
-          chunks.push(chunk);
-        });
-  
-        response.on('end', () => {
-          resolve({
-            body: Buffer.concat(chunks).toString(),
-            status: response.statusCode
-          });
-        });
-      }).on('error', reject);
-    }, waitFor || 0);
-  });
+    return new Promise((resolve, reject) => {
+        setTimeout(() => {
+            http.get(url, (response) => {
+                var chunks = [];
+                response.on('data', (chunk) => {
+                    chunks.push(chunk);
+                });
+
+                response.on('end', () => {
+                    resolve({
+                        body: Buffer.concat(chunks).toString(),
+                        status: response.statusCode
+                    });
+                });
+            }).on('error', reject);
+        }, waitFor || 0);
+    });
 }
 
 /**
@@ -103,24 +104,24 @@ function triggerEndpoint(url, waitFor) {
  * @param {string} expectedFields.url Express route url
  */
 function validateExpressSegment(segment, expectedFields) {
-  assert.equal(segment.name, expectedFields.name);
-  assert.isNumber(segment.end_time);
-  assert.isNumber(segment.start_time);
-  assert.equal(segment.http.request.method, 'GET');
-  assert.isString(segment.http.request.client_ip);
-  assert.equal(segment.http.request.url, expectedFields.url);
-  assert.equal(segment.http.response.status, expectedFields.responseStatus);
-  assert.isString(segment.trace_id);
+    assert.equal(segment.name, expectedFields.name);
+    assert.isNumber(segment.end_time);
+    assert.isNumber(segment.start_time);
+    assert.equal(segment.http.request.method, 'GET');
+    assert.isString(segment.http.request.client_ip);
+    assert.equal(segment.http.request.url, expectedFields.url);
+    assert.equal(segment.http.response.status, expectedFields.responseStatus);
+    assert.isString(segment.trace_id);
 }
 
 module.exports = {
-  createAppWithRoute: createAppWithRoute,
-  createDaemon: createDaemon,
-  jitter: jitter,
-  messageCounter: messageCounter,
-  parseMessage: parseMessage,
-  sleep: sleep,
-  sleepDedupe: sleepDedupe,
-  triggerEndpoint: triggerEndpoint,
-  validateExpressSegment: validateExpressSegment
+    createAppWithRoute: createAppWithRoute,
+    createDaemon: createDaemon,
+    jitter: jitter,
+    messageCounter: messageCounter,
+    parseMessage: parseMessage,
+    sleep: sleep,
+    sleepDedupe: sleepDedupe,
+    triggerEndpoint: triggerEndpoint,
+    validateExpressSegment: validateExpressSegment
 };
\ No newline at end of file