Add bind option to getCurrentContext()

Add bind:true option to getCurrentContext

Workaround issue #17 with cujojs/when v3.7.7 due to the fact
that it internally uses a mechanism of user-land queuing for
async-related operations, and possibly issues with similar
packages using queuing (mostly Promise libraries)

README.md

@@ -8,10 +8,15 @@ Current context for LoopBack applications, based on cls-hooked.
 
 ### Known issues
 
- - [when](https://www.npmjs.com/package/when), a popular Promise
+- [when](https://www.npmjs.com/package/when), a popular Promise
    implementation, breaks context propagation. Please consider using the
    built-in `Promise` implementation provided by Node.js or
    [Bluebird](https://www.npmjs.com/package/bluebird) instead.
+- Express middleware chains which contain a "bad" middleware (i.e. one which
+  breaks context propagation inside its function body, in a way mentioned in
+  this doc) especially if called before other "good" ones needs refactoring,
+  in order to prevent the context from getting mixed up among HTTP requests.
+  See usage below for details.
 
    Discussion: https://github.com/strongloop/loopback-context/issues/17
 
@@ -62,7 +67,6 @@ This approach should be compatible with all process managers, including
 `strong-pm`. However, we feel that relying on the order of `require` statements
 is error-prone.
 
-
 ### Configure context propagation
 
 To setup your LoopBack application to create a new context for each incoming
@@ -114,6 +118,48 @@ MyModel.myMethod = function(cb) {
 });
 ```
 
+### Bind for concurrency
+
+In order to workaround the aforementioned concurrency issue with `when` (and
+similar `Promise`-like and other libraries implementing custom queues and/or
+connection pools), it's recommended to activate context binding inside each
+HTTP request or concurrent `runInContext()` call, by using the `bind` option, as
+in this example:
+
+    var ctx = LoopBackContext.getCurrentContext({ bind: true });
+
+With the option enabled, this both creates the context, and binds the access
+methods of the context (i.e. `get` and `set`), at once.
+
+**Warning**: this only works if it's **the first expression evaluated** in every
+middleware/operation hook/`runInContext()` call etc. that uses
+`getCurrentContext`. (It must be the first expression; it may not be enough if
+it's at the first line). Explanation: you must bind the context while it's still
+correct, i.e. before it gets mixed up between concurrent operations affected by
+bugs. Therefore, to be sure, you must bind it before *any* operation.
+
+Also, with respect to the "bad", context-breaking middleware use case mentioned in "Known issues"
+before, the following 2 lines need to be present at the beginning of the middleware
+body. At least the "bad" one; but, as a preventive measure, they can be present
+in every other middleware of every chain as well, being backward-compatible:
+
+     var badMiddleware = function(req, res, next) {
+       // these 2 lines below are needed
+       var ctx = LoopBackContext.getCurrentContext({bind: true});
+       next = ctx.bind(next);
+       ...
+
+The `bind` option defaults to `false`. This is only in order to prevent breaking
+legacy apps; but if your app doesn't have such issue, then you can safely use
+`bind: true` everywhere in your app (e.g. with a
+[codemod](https://github.com/facebook/jscodeshift), or by monkey-patching
+`getCurrentContext()` globally, if you prefer an automated fashion).
+
+**Warning**: this only applies to application modules. In fact, if the module
+affected by the concurrency issue is of this kind, you can easily refactor/write
+your own code so to enable `bind`. Not if it's a 3rd-party module, nor a
+Loopback non-core module, unless you fork and fix it.
+
 ### Use current authenticated user in remote methods
 
 In advanced use cases, for example when you want to add custom middleware, you

package.json

@@ -26,13 +26,14 @@
     "cls-hooked": "^4.0.1"
   },
   "devDependencies": {
-    "async": "1.5.2",
+    "async-1.5.2": "file:./test/stub-modules/async-1.5.2",
     "chai": "^3.5.0",
     "dirty-chai": "^1.2.2",
     "eslint": "^2.13.1",
     "eslint-config-loopback": "^4.0.0",
     "loopback": "^3.0.0",
     "mocha": "^2.5.3",
-    "supertest": "^1.2.0"
+    "supertest": "^1.2.0",
+    "when-3.7.7": "file:./test/stub-modules/when-3.7.7"
   }
 }

server/current-context.js

@@ -14,9 +14,13 @@ var LoopBackContext = module.exports;
  * Get the current context object. The context is preserved
  * across async calls, it behaves like a thread-local storage.
  *
+ * @options {Object} [options]
+ * @property {Boolean} bind Bind get/set/bind methods of the context to the
+ * context that's current at the time getCurrentContext() is invoked. This
+ * can be used to work around 3rd party code breaking CLS context propagation.
  * @returns {Namespace} The context object or null.
  */
-LoopBackContext.getCurrentContext = function() {
+LoopBackContext.getCurrentContext = function(options) {
   // A placeholder method, see LoopBackContext.createContext() for the real version
   return null;
 };
@@ -77,8 +81,27 @@ LoopBackContext.createContext = function(scopeName) {
     ns = cls.createNamespace(scopeName);
     process.context[scopeName] = ns;
     // Set up LoopBackContext.getCurrentContext()
-    LoopBackContext.getCurrentContext = function() {
-      return ns && ns.active ? ns : null;
+    LoopBackContext.getCurrentContext = function(options) {
+      if (!ns || !ns.active) {
+        return null;
+      }
+      if (!options || !options.bind) {
+        return ns;
+      }
+      /**
+       * **NOTE**
+       * This only re-binds get, set and bind methods, the most used.
+       * If you use other methods of the context, e.g. runInContext(), etc.,
+       * you may run into unexpected issues that are fixed only for get & set.
+       */
+      var boundContext = Object.create(ns);
+      boundContext.get = boundContext.bind(ns.get);
+      boundContext.set = boundContext.bind(ns.set);
+
+      // Call to Function.prototype.bind(), not ns.bind()
+      boundContext.bind = ns.bind.bind(ns);
+
+      return boundContext;
     };
   }
   return ns;

test/main.test.js

@@ -5,7 +5,8 @@
 
 'use strict';
 
-var async = require('async');
+var asyncV152 = require('async-1.5.2');
+var whenV377 = require('when-3.7.7');
 var LoopBackContext = require('..');
 var Domain = require('domain');
 var EventEmitter = require('events').EventEmitter;
@@ -106,10 +107,9 @@ describe('LoopBack Context', function() {
   // Heavily edited by others
   it('keeps context when using waterfall() from async 1.5.2',
   function(done) {
-    expect(require('async/package.json').version).to.equal('1.5.2');
     LoopBackContext.runInContext(function() {
       // Trigger async waterfall callbacks
-      async.waterfall([
+      asyncV152.waterfall([
         function pushToContext(next) {
           var ctx = LoopBackContext.getCurrentContext();
           expect(ctx).is.an('object');
@@ -129,4 +129,121 @@ describe('LoopBack Context', function() {
       ], done);
     });
   });
+
+  it('handles concurrent then() calls with when v3.7.7 promises & bind option',
+  function() {
+    return Promise.all([
+      runWithPushedValue('test-value-1', {bind: true}),
+      runWithPushedValue('test-value-2', {bind: true}),
+    ])
+    .then(function verify(values) {
+      var failureCount = getFailureCount(values);
+      expect(failureCount).to.equal(0);
+    });
+  });
+
+  it('fails once without bind option and when v3.7.7 promises',
+  function() {
+    return Promise.all([
+      runWithPushedValue('test-value-3'),
+      runWithPushedValue('test-value-4'),
+    ])
+    .then(function verify(values) {
+      var failureCount = getFailureCount(values);
+      expect(failureCount).to.equal(1);
+    });
+  });
+
+  var timeout = 100;
+
+  function runWithPushedValue(pushedValue, options) {
+    return new Promise(function concurrentExecutor(outerResolve, reject) {
+      LoopBackContext.runInContext(function pushToContext() {
+        var ctx = LoopBackContext.getCurrentContext(options);
+        expect(ctx).is.an('object');
+        ctx.set('test-key', pushedValue);
+        var whenPromise = whenV377().delay(timeout);
+        whenPromise.then(function pullFromContextAndReturn() {
+          var pulledValue = ctx && ctx.get('test-key');
+          outerResolve({
+            pulledValue: pulledValue,
+            pushedValue: pushedValue,
+          });
+        }).catch(reject);
+      });
+    });
+  }
+
+  function getFailureCount(values) {
+    var failureCount = 0;
+    values.forEach(function(v) {
+      if (v.pulledValue !== v.pushedValue) {
+        failureCount++;
+      }
+    });
+    return failureCount;
+  }
+
+  it('doesn\'t mix up req\'s in chains of ' +
+  'Express-middleware-like func\'s if next() cb is bound',
+  function() {
+    return Promise.all([
+      runWithRequestId('test-value-5', true),
+      runWithRequestId('test-value-6', true),
+    ])
+    .then(function verify(values) {
+      var failureCount = getFailureCount(values);
+      expect(failureCount).to.equal(0);
+    });
+  });
+
+  it('fails & mixes up ctx among requests in mw chains if next() cb is unbound',
+  function() {
+    return Promise.all([
+      runWithRequestId('test-value-7'),
+      runWithRequestId('test-value-8'),
+    ])
+    .then(function verify(values) {
+      var failureCount = getFailureCount(values);
+      expect(failureCount).to.equal(1);
+    });
+  });
+
+  function runWithRequestId(pushedValue, bindNextCb) {
+    return new Promise(function chainExecutor(outerResolve, reject) {
+      LoopBackContext.runInContext(function concurrentChain() {
+        function middlewareBreakingCls(req, res, next) {
+          var ctx = LoopBackContext.getCurrentContext({bind: true});
+          if (bindNextCb) {
+            next = ctx.bind(next);
+          }
+          ctx.set('test-key', pushedValue);
+          var whenPromise = whenV377().delay(timeout);
+          whenPromise.then(next).catch(reject);
+        };
+
+        function middlewareReadingContext(req, res, next) {
+          var ctx = LoopBackContext.getCurrentContext({bind: true});
+          var pulledValue = ctx && ctx.get('test-key');
+          next(null, pulledValue);
+        };
+
+        // Run the chain
+        var req = null;
+        var res = null;
+        middlewareBreakingCls(req, res, function(err) {
+          if (err) return reject(err);
+
+          middlewareReadingContext(req, res, function(err, result) {
+            if (err) return reject(err);
+
+            outerResolve({
+              pulledValue: result,
+              pushedValue: pushedValue,
+            });
+          });
+        });
+      });
+    });
+  }
 });

test/stub-modules/async-1.5.2/index.js

@@ -0,0 +1,2 @@
+'use strict';
+module.exports = require('async');

test/stub-modules/async-1.5.2/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "async-1.5.2",
+  "version": "1.5.2",
+  "description":"async version 1.5.2",
+  "dependencies": {
+    "async":"1.5.2"
+  }
+}

test/stub-modules/when-3.7.7/index.js

@@ -0,0 +1,2 @@
+'use strict';
+module.exports = require('when');

test/stub-modules/when-3.7.7/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "when-3.7.7",
+  "version": "3.7.7",
+  "description":"when version 3.7.7",
+  "dependencies": {
+    "when":"3.7.7"
+  }
+}