timbeyer/cls-bluebird

cls-bluebird's Issues

LICENSE file

I'd like to use this library, but I can't legally use it unless it has a LICENSE file in it. I don't suppose you folks can add that in so I can cross off my red box?

Change approach to how this patch works internally

I've been working on a new set of tests. Here it is (work in progress): https://github.com/overlookmotel/cls-bluebird/tree/tests-rewrite

And the test results on Travis: https://travis-ci.org/overlookmotel/cls-bluebird/jobs/140228913

As you can see, it's getting a bit out of control! There are so many different combinations of:

• methods
• promises resolved synchronously or asynchronously
• promises created and methods called in different CLS contexts
• promises resolved from a variety of other thenables

There's already 250 tests, and that's purely for Promise.resolve() and Promise.reject() with bluebird v2 - haven't written tests for any of the other methods yet! I'm also not sure that I've covered every possible case.

I've come to the conclusion that this is the wrong way to go about it.

The problem

The current patch simply patches internal bluebird method promise.prototype._addCallbacks to bind callbacks to the current CLS namespace. The logic is that every bluebird method that adds a callback to a promise calls _addCallbacks internally, so the callbacks are always bound.

This is really smart and elegant, but it relies on the above assumption.

In bluebird v3 the internals of bluebird changed so that _addCallbacks is bypassed when a calling .then() on a promise that's already resolved. So that assumption was no longer correct, and it broke the cls-bluebird patch.

I've also just found a bug with Promise.reduce() in bluebird v2 (issue #12) - again, Promise.reduce() doesn't call _addCallbacks.

A proposal

I suggest that a better approach would be to patch each of bluebird's public methods individually, much like Sequelize's bluebird/CLS shim does.

Why?

This is much less clever and not so elegant, but it does have the following advantages:

1. Internal changes to bluebird in future versions would not break the patch (NB internal changes that don't affect the public API could happen in even a patch release).
2. More robust - less likely that there's weird edge cases where whatever assumptions the patch is based on aren't correct.
3. Tests can be much simpler and cover all cases with greater reliability.

The tests would only have to cover two areas:

1. Every public method binds all callbacks which are executed asynchronously.
2. Every public method returns a promise which is an instance of the patched bluebird constructor (i.e. Bluebird.resolve( Q.resolve() ) returns a Bluebird promise not a Q promise).

I think this would make it possible to write a set of tests that you can be confident really do cover all cases.

What's the downside?

It's possible that some callbacks will be bound unnecessarily.

An example: Promise.map( arr, fn ) executes fn synchronously if arr is an array of literal values. If it's an array of promises, however, fn is executed async after the promises resolve.

To cover the latter case, fn would have to be bound, but that's unnecessary when it ends up being called synchronously. So it's a small performance hit.

Why does this matter so much anyway?

My feeling is for CLS "mostly works" isn't good enough. You need to have 100% confidence that it's completely bullet-proof.

As @iliakan said in othiym23/node-continuation-local-storage#59, the consequences of CLS contexts getting mixed up between requests on an HTTP server can be quite dire e.g. a regular user getting admin privileges.

So, in my opinion, even if my proposal is a lot less "nice" and also slightly less performant, it's worth it for the increased reliability.

@TimBeyer what do you think?

Not working with bluebird v3

Raising this issue more to warn others than anything else at this point.

cls-bluebird does not currently work with bluebird v3.

There are at least a couple of problems:

1. The progress argument has been removed from _addHandlers() in bluebird v3
2. When calling .then() on promise which is already resolved, _addHandlers() is not called in bluebird v3

I'm working on fixing this, but at present best to avoid using this module with bluebird versions later than 2.x.x

Broke with Bluebird 3.5.1

I recently upgraded Bluebird from 3.5.0 to 3.7.1. Cls patching didn't work anymore. Tracked it back to the version change from 3.5.0 where it was still fine to the change to 3.5.1, where it broke.

cls and promisify

Hi Tim,
I'm having troubles using async/await and cls on a library that has been promisified with bluebird.
The particular library is
https://github.com/maxlath/nano-blue2
but maybe the solution is valid in general.

Can you please point me in the right direction?
Thanks a lot!

how to run test?

I'd like to submit a patch to fix cls with Promise.coroutine. However, regression cannot be done because I cannot run unit tests.

npm test seemed to have no response until I sent SIGINT.

Of course, I already had a redis instance running at localhost:6379.

Enable Travis

Hi @TimBeyer.

Thanks loads for making me a contributor. I'll aim to do you proud!

Could you possibly enable Travis for this repo? I can't do it - it seems only the repo owner can.

I'd like to run the tests against different versions of node, and it's also useful to have the test result history public, and see if any PRs break the tests.

I see you have a Travis account, so it should be as easy as going to https://travis-ci.org/TimBeyer/cls-bluebird and clicking "Activate repository".

await is not working :(

(async () => {
  await new Promise((resolve) => {
    ns.run(() => {
      ns.set('name', 'tom')
      return resolve()
    })
  })
  console.log(ns.get('name'))
})()

RangeError: Maximum call stack size exceeded

We are using Sequeilze which internally uses bluebird. To fix the logging so that it has session id which connects all our logs in one request we added cls-bluebird. But now we are seeing that we are getting "RangeError: Maximum call stack size exceeded" after our application runs for few hours.

What may cause this and is this a know issue?

Clarifying purpose of cls-bluebird

After delving into how promises and CLS interact, I've come to the conclusion that there are 3 different ways in which a CLS/Promise shim can work.

All three approaches have their own logic, and they're all incompatible with each other.

It's not clear which is the "correct" way. I wondered if we could discuss, and clarify which approach cls-bluebird is aiming for.

Apologies that this is rather long, but there are some subtleties at play here, and it's hard to tease it out.

The 3 conventions

Here are the 3 different approaches:

Convention 1: Callback style

This is the behavior of native JS Promises.

The context at the end of the last .then() callback is maintained for the next .then() callback. Where and when the .then() is added to the promise chain is irrelevant.

For CLS purposes, the following are treated the same:

fs.readFile('foo.txt', function(text) {
    console.log(text);
});

fs.readFile('foo.txt').then(function(text) {
    console.log(text);
});

i.e. promises are essentially sugar for callbacks, rather than a distinct syntax with different behavior.

If the code inside a .then() callback loses CLS context (due to using a queue or similar), then the shim would NOT correct this.

On the positive side, it allows a CLS context to be created within a .then() callback and the rest of the promise chain that follows runs within that context. This could be useful e.g. for middleware.

Promise.resolve().then(function() {
    return new Promise(function(resolve) {
        ns.run(function() {
            ns.set('foo', 123);
            resolve();
        });
    });
}).then(function() {
    console.log(ns.get('foo')); // prints 123
});

Convention 2: Follow promise chain

CLS context is set at the time of the promise's creation. Any promises which chain on from another promise inherit the same context.

This is the same as (1) except:

• If a .then() callback loses context, context is restored for the next .then() callback
• If a new CLS context is created within a .then() callback, it is NOT maintained for the next .then() in the promise chain

ns.run(function() {
    ns.set('foo', 123);
    Promise.resolve().then(function() {
        return loseContext(); // returns a promise, but loses CLS context
    }).then(function() {
        // original CLS context has been restored
        console.log(ns.get('foo')); // prints 123
    });
});

var promise;
ns.run(function() {
    ns.set('foo', 123);
    promise = Promise.resolve();
});

ns.run(function() {
    ns.set('foo', 456);
    promise.then(function() {
        console.log(ns.get('foo')); // prints 123
    });
});

Convention 3: Listener attachment context

CLS context for execution of .then() callback is defined at time .then() is called. This is not necessarily the same context as the previous promise in the chain.

Similarly to (2), if a .then() callback loses context, this doesn't affect context for the next .then() in the chain.

This appears to be the convention followed by cls-bluebird at present.

var promise;
ns.run(function() {
    ns.set('foo', 123);
    promise = Promise.resolve();
});

ns.run(function() {
    ns.set('foo', 456);
    promise.then(function() {
        console.log(ns.get('foo')); // prints 456
    });
});

Difference between the three

The following code demonstrates the difference between the 3 conventions. It will log "This Promise implementation follows convention X", where X depends on which approach the promise shim takes.

var promise;
ns.run(function() {
    ns.set('test', 2);
    promise = new Promise(function(resolve) {
        ns.run(function() {
            ns.set('test', 1);
            resolve();
        });
    });
});

ns.run(function() {
    ns.set('test', 3);
    promise.then(function() {
        console.log('This Promise implementation follows convention ' + ns.get('test'));
    });
});

NB With native JS promises you get "This Promise implementation follows convention 1". With cls-bluebird you get "This Promise implementation follows convention 3".

Which way is best?

I think this is debatable. It depends on how you conceptualize promises and the control flow they represent.

Convention 1 is the simplest and isn't opinionated about what a promise control flow represents.

Native JS Promises follow this convention, so there's an argument other promise shims should follow the same convention to avoid confusion.

This doesn't cover the common use case of patching where a library like redis loses CLS context within it. However, there's a strong separation of concerns argument that a shim for a promise library should just shim the promise library. If another library loses CLS context, then that library should be shimmed. i.e. solve the problem that redis loses context with cls-redis not cls-bluebird!

Convention 2 conceptualizes a promise chain as a set of connected actions.

Imagine multiple tasks running in parallel, each composed of multiple steps e.g. read a file, transform it, write it out again. Each task run is represented by a promise chain.

Now if you want to add an extra step to each of the tasks (e.g. notify a server when task is done), you'd add an extra .then() to the end of the promise chain for each task. You would expect each callback to run in the CLS context for that task.

Convention 3 conceptualizes a promise chain as a queue.

Imagine a resource which can only be accessed by one process at a time. The queue for access is represented by a promise. When a process finishes accessing the resource, it resolves the promise and the next in the queue (next promise in the chain) then starts up. If you want access to the resource, you add a .then() to the promise chain.

If a running task (e.g. serving an HTTP request), gets the resource and then continues on with other things, you would expect promises chained on after accessing the resource to execute in the CLS context of the task, NOT the context of the preceding item in the resource queue.

function Resource() {
    this.promise = Promise.resolve();
}

Resource.prototype.read = function() {
    this.promise = this.promise.then(function() {
        return fs.readFileAsync('/path/to/resource'); // NB returns promise
    });
    return this.promise;
};

var resource = new Resource();

// somewhere else in code
function doTheDo() {
    return resource.read().then(function(resourceContent) {
        // do something with the resource's content
    });
}

Conclusion

I'm not pushing for one convention over another. I just thought it'd be useful to lay out what I think are the 3 different choices and their implications.

What I do suggest is that cls-bluebird should explicitly choose which convention it follows and document that, to avoid confusion. This would also make it possible to decide whether any given behavior is correct or a bug (there's been some ambiguity for example in discussion in #1).

What are your thoughts?

`Promise.reduce()` broken in bluebird v2

Promise.reduce() loses CLS context with bluebird v2.

Example:

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

var Promise = require('bluebird');
require('cls-bluebird')(ns);

ns.run(function() {
    ns.set('id', 1);
    Promise.reduce([1, 2, 3], function(total, v) {
        console.log('In context 1:', ns.get('id'));
    }, 0);
});

ns.run(function() {
    ns.set('id', 2);
    Promise.reduce([1, 2, 3], function(total, v) {
        console.log('In context 2:', ns.get('id'));
    }, 0);
});

Expected output is:

In context 1: 1
In context 1: 1
In context 1: 1
In context 2: 2
In context 2: 2
In context 2: 2

But in reality you get:

In context 1: 1
In context 1: 1
In context 1: 1
In context 2: 1
In context 2: 1
In context 2: 1

So CLS context is getting mixed up between the two reduce callbacks.

Does cls-bluebird also work with cls-hooked?

cls-bluebird works for continuation-local-storage as mentioned in the readme.

Does cls-bluebird also work with cls-hooked? Sequelize does not support continuation-local-storage anymore, but only cls-hooked.
It does not crash and seems to work, but I have no idea about the internals of cls-hooked and continuation-local-storage.

Re-write of tests

Hi @TimBeyer.

Would you likely accept a PR completely rewriting the tests? Here's what I have in mind:

• using mocha
• removing the redis dependency (using lose-cls-context instead)
• testing against both bluebird v2 and v3
• testing every conceivable combination of methods and circumstances (including those mentioned as potentially problematic in #1)

Apologies for the sudden torrent of issues! Sequelize is switching to using co-routines internally and I want to take this opportunity to move it over to using cls-bluebird for its CLS support. But we need to make sure it's as robust as possible as CLS maintains transactions in Sequelize and so is completely critical to ensuring data consistency in the database. Hence a desire to really flesh out the tests to cover all possible cases.

I'm happy to do a lot of legwork myself on this (and it will be quite an effort). But just want to make sure you're going to be willing and have time to review PRs.