othiym23 / node-continuation-local-storage

implementation of https://github.com/joyent/node/issues/5243
BSD 2-Clause "Simplified" License
1.13k stars 207 forks source link

Namespace remains active if error is thrown in catch of Promise #119

Open apalsapure opened 7 years ago

apalsapure commented 7 years ago

I'm using this module in my express app. As you suggested I have used it as a request context and stopped setting properties on request and response. But I ran into a problem. When an error is thrown inside Promise, active context(namespace) never exits. All the calls made after this; share same context.

Following is a small code snippet which shows the issue.

var express = require('express')
var app = express();
var getNamespace = require('continuation-local-storage').getNamespace;
var nameSpaceName = 'sample';
var sampleKey = 'sampleKey';

var createNamespace = require('continuation-local-storage').createNamespace;
createNamespace(nameSpaceName);

// add middle-ware
app.use(function (req, res, next) {
  var namespace = getNamespace(nameSpaceName);

  // hack for exiting context <=========
  if (namespace.active) {
    console.log(namespace.get(sampleKey)); // prints `sample` instead of `undefined`
    console.log('calling exit');
    namespace.exit(namespace.active);
  }

  namespace.bindEmitter(req);
  namespace.bindEmitter(res);

  namespace.run(function () {
    next();
  });
});

// throws an error in promise
var errorPromise = function () {
  var promise = new Promise((resolve, reject) => {
    reject('str');
  });
  Promise.resolve(promise).then(function (obj) {
  }).catch(function (err) {
    throw new Error(err);
  });
}

// route
app.get('/', function (req, res) {
  // adding sample key in context
  // which should reset in every call
  var namespace = getNamespace(nameSpaceName);
  namespace.set(sampleKey, 'sample');

  // if request query as test, call promise
  if (req.query.test)
    errorPromise();

  res.status(200).send('{"status": 200}');
});
app.listen(3000, function () {
  console.log('Example app listening on port 3000!')
})

Here when query string contains test, call to error promise is made. After this error when next call is made, the earlier context is still active.

Following are the requests made and respective console output

http://localhost:3000/
http://localhost:3000/?test=true
http://localhost:3000/

output:
Example app listening on port 3000!
(node:796) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): Error: str
sample
calling exit

For now before I call namespace.run, if the namespace is active, I explicitly call exit on it. But this is not the right solution, as in parallel calls for few milliseconds the context is shared.

Please let me know if I'm doing something wrong or propose a solution.

Thanks.

heycalmdown commented 7 years ago

I don't understand why this happens, but here is an ad-hoc for this.

var errorPromise = function () {
  var promise = new Promise((resolve, reject) => {
    reject('str');
  });
  Promise.resolve(promise).then(function (obj) {
  }).catch(function (err) {
    //throw new Error(err);
    return Promise.reject(err);
  });
}
carlisliu commented 7 years ago

Actually, I think it's a async-listener problem, not continuation-local-storage. In this case, when throw an error in promise.catch method, it trigger 'unhandledRejection' instead of calling process._fatalException function. Missing calling for asyncCatcher