Refactor CLS support

[overlookmotel]

Hi all.

I've been investigating the reason why sometimes my code is losing CLS context when combining Sequelize with co.

I think that the cause is the way CLS is handled within Sequelize. This is a bit subtle, so I'm going to lay it out at length...

There are two problems that need to be solved to support CLS:

1. bluebird loses CLS context
2. Certain function calls deep within Sequelize lose CLS context - notably the database libraries and generic-pool

The present solution is that all bluebird methods (.then() etc) are shimmed so that the callbacks are all bound to the CLS context. (see https://github.com/sequelize/sequelize/blob/master/lib/promise.js)

This works almost 100%, however, there's a subtle problem. The CLS context that the .then() callback receives is the context when .then() is called rather than maintaining the context of the previous call in the promise chain.

For example:

var cls = require('continuation-local-storage');
var Sequelize = require('sequelize');
var namespace = cls.createNamespace('myApp');
Sequelize.cls = namespace;

var sequelize = new Sequelize(/* connection params */);

var User = sequelize.define('User', { name: Sequelize.STRING(100) });

var promise;
namespace.run(function() {
    namespace.set('test', 123);

    promise = User.findAll().then(function() {
        console.log('1st value:', namespace.get('test'));
    });
});

promise.then(function() {
    console.log('2nd value:', namespace.get('test'));
});

The output is:

1st value: 123
2nd value: undefined

My feeling is that the 2nd .then() callback should be called within the CLS context of the promise chain and therefore should output 2nd value: 123.

I think this is also the reason why CLS context also gets lost sometimes when yielding a Sequelize promise within a co co-routine.

I propose an alternative way of supporting CLS within Sequelize which would resolve this problem:

1. Remove the shims on bluebird methods
2. Use cls-bluebird to patch bluebird so it maintain CLS context
3. In Sequelize codebase, manually re-bind CLS context after calls to database libraries etc which lose context

I am happy to implement this myself, but just wanted to check if it would be a welcome PR.

Also, I guess this could in some few edge cases break peoples' apps, so would be best to go into Sequelize v4. What's the timescale for release of v4 looking like?

[mickhansen]

Having the context be same for the entire promise chain would definitely be ideal.
We currently looking at defining all the changes we want for v4, you can look at the v4.0 milestone for an idea.

[overlookmotel]

On second thoughts, I don't think this change should be considered a breaking change, but rather a bug fix.

Changing the example above to either promise = Promise.resolve(User.findAll()) or promise = Promise.resolve() (where Promise is the native JS Promise) gives output 2nd value: 123.

So I conclude that what I'm suggesting is the correct behavior. So I guess it wouldn't need to wait for v4.

[overlookmotel]

@mickhansen OK cool. I'll get to work on it when I can.

[janmeier]

The context should definitely be preserved for the whole lifetime of the promise chain

I'm not sure we can use cls-bluebird instead of our own shimming - we need to manually shim all the methods, if we only shim then the context is still lost for example with a call to spread.

[overlookmotel]

@janmeier cls-bluebird patches bluebird so CLS context is preserved across all bluebird methods - .then, .spread, .catch etc. I referenced only .then above just as a short hand. Apologies for the confusion.

[overlookmotel]

OK, this is going to take some time. I'm going to start by re-writing all the tests for cls-bluebird to clarify where context should be maintained and where it shouldn't, and then making sure it's doing what it should be.

But watch this space...

[overlookmotel]

For anyone who's interested, here's where I'm up to so far: https://github.com/overlookmotel/cls-bluebird-test

[overlookmotel]

Having looked into this fairly deeply, I've decided that the issue I raised here isn't a bug per se. There are several approaches to how CLS should work with promises, and I've decided that the best thing is to maintain the convention that callbacks should continue the CLS context from when the handler is attached to the promise.

This is "convention 3" as discussed in this issue, which has generated a lot of interesting discussion: othiym23/node-continuation-local-storage#64

Therefore closing this issue.