Handling asynchronous initialization and promises

[matthewloring]

Asynchronous initialization presents a problem for many node modules. One common pattern for ensuring initialization is only done once is to wrap initialization in a promise that is "thened" on each module operation:

var initialize = new Promise(function(resolve, reject) {
  resolveMetadata(function(err, response) {
    if (err) reject(err);
    resolve(response.metadata);
  });
});

function foo(callback) {
  initialize.then(function(metadata) {
    doOperation(metadata, callback);
  });
}

module.exports = foo;

Because continuation-local-storage propagates context from a promise's creation context into the then function, this will cause each call to foo made from inside a namespace#run to lose context. This is problematic if a modules is trying to handle each incoming request to a web server in a separate namespace and foo is invoked per request.

This small test case demonstrates the issue:

var cls = require('continuation-local-storage');
var ns = cls.createNamespace('foo');

var p = new Promise(function(res, rej) {
  res(5);
});

ns.run(function() {
  ns.set('a', 'b');
  p.then(function() {
    console.log(ns.get('a')); // prints undefined but should print 'b'
  });
});

It would be great if then only ran in p's context if there was a context at the time p was created. To play around with this issue, I tried this async-listener patch:

diff --git a/index.js b/index.js
index e634ac4..51649bd 100644
--- a/index.js
+++ b/index.js
@@ -490,14 +490,14 @@ function wrapPromise() {
       // continuations of the accept or reject call using the __asl_wrapper created above.
       function bind(fn) {
         if (typeof fn !== 'function') return fn;
-        return function (val) {
+        return wrapCallback(function (val) {
           var result = (promise.__asl_wrapper || propagateAslWrapper)(this, fn, val, next);
           if (result.error) {
             throw result.errorVal
           } else {
             return result.returnVal
           }
-        };
+        });
       }
     }
   }

This patch will result in the above code snippet printing 'b' without clobbering context if then is called at point with no active namespace:

var cls = require('continuation-local-storage');
var ns = cls.createNamespace('foo');

var p2;

ns.run(function() {
  ns.set('c', 'd');
  p2 = new Promise(function(res, rej) {
    res(6);
  });
});

ns.run(function() {
  ns.set('c', 'e');
  p2.then(function() {
    console.log(ns.get('c')); // prints 'd' with and without patch
  });
});

While this change (or similar) will not address all issues CLS has with promises, it will reduce the amount of patching APM vendors need to do for this relatively common pattern. What do others think about a change like this? /cc @Qard @watson @ofrobots

[watson]

Thanks for the ping. We don't use the CLS module in the Opbeat node agent, so we are not directly affected by this (though I can't rule out that we're not affected by something similar).

I'm interested in the discussion nonetheless, though I can't really say much about this scenario off the top of my head - so I'll look forward to hear feedback from others.

[Qard]

Seems reasonable.

When I was still at AppNeta, I was actually considering a slightly different change of tracking context at both internal promise resolution and at the point then() is called. Then the context set inside the callback given to then() would use one of the two potential contexts but instead prioritizing the then() call context over the internal resolution context, that way it'd tend toward following the user code over the internals.

This seems like an easier change though with less chance of breaking things.

[matthewloring]

I also played around with tracking both the resolution and chaining context but found that approach to be more error prone and susceptible to "but given user intent the other context is better" issues. I tried running the async-listener tests with this change and there seemed to be many failures as the tests expect a particular trace of async events to have occurred. Even though this change will only introduce context where there was previously none, it does modify these traces causing tests to fail. Would you be open to changing the expected traces in these tests? If so, I can open a pr with this patch + test changes.

[Qard]

I'm fine with that. Not sure if @othiym23, @hayes or @watson have any concerns.

I've never really liked the tests in async-listener--too much emphasis on testing of internal behaviour over how it should actually work from the user perspective.

[hayes]

I think for most cases this would be fine, the place where I would be concerned about this would if there are things that someone would want to explicitly exclude from a namespace by wrapping them with "no state". In those cases things that were previously excluded would now potentially be included again. That being said, I can't come up with a compelling example of user code that would actually include things one would expect to be excluded. I think this proposal is reasonable, and would be a net positive.

I don't really have a stake in this anymore since I don't work on node apm these days

[Qard]

Yeah...it seems like very few of us do. 😕

[hayes]

also I apologize for the state of those tests, I always hated them as well, but was too lazy/busy to come up with something better. In the long run, I think that the instrumentation around promises will need to be rethought entirely to support async/await, but for now, I don't think there should be much resistance to just updating the tests to match the new semantics

[Qard]

No worries. It's a hard and constantly evolving problem. None of us had the time to spend perpetually chasing perfection.

I just want things to keep moving forward. 😸

[matthewloring]

Fixed by https://github.com/othiym23/async-listener/pull/105. Thanks all!